You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2021/07/26 16:19:21 UTC
[GitHub] [apisix] nanamikon opened a new pull request #4676: feat: support dash in args (#4519)
nanamikon opened a new pull request #4676:
URL: https://github.com/apache/apisix/pull/4676
### What this PR does / why we need it:
<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here. -->
Support fetch value from arg name with dash , see #4519
### Pre-submission checklist:
* [ 1] Did you explain what problem does this PR solve? Or what new features have been added?
* [ 1] Have you added corresponding test cases?
* [ 1] Have you modified the corresponding document?
* [ 1] Is this PR backward compatible? **If it is not backward compatible, please discuss on the [mailing list](https://github.com/apache/apisix/tree/master#community) first**
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on pull request #4676: feat: support dash in args (#4519)
Posted by GitBox <gi...@apache.org>.
spacewander commented on pull request #4676:
URL: https://github.com/apache/apisix/pull/4676#issuecomment-892278445
@nanamikon
Yes, let's do it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] nanamikon commented on a change in pull request #4676: feat: support dash in args (#4519)
Posted by GitBox <gi...@apache.org>.
nanamikon commented on a change in pull request #4676:
URL: https://github.com/apache/apisix/pull/4676#discussion_r677053635
##########
File path: apisix/core/ctx.lua
##########
@@ -159,10 +160,22 @@ do
val, err = cookie:get(sub_str(key, 8))
if err then
log.warn("failed to fetch cookie value by key: ",
- key, " error: ", err)
+ key, " error: ", err)
Review comment:
ok
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #4676: feat: support dash in args (#4519)
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #4676:
URL: https://github.com/apache/apisix/pull/4676#discussion_r678750585
##########
File path: apisix/core/ctx.lua
##########
@@ -159,10 +160,22 @@ do
val, err = cookie:get(sub_str(key, 8))
if err then
log.warn("failed to fetch cookie value by key: ",
- key, " error: ", err)
+ key, " error: ", err)
end
end
+ elseif core_str.has_prefix(key, "arg_") then
+ local args = t.args
+ local arg_key = sub_str(key, 5)
+ local m, err = ngx_re_match(args, "(^|&)" .. arg_key .. "=([^&]*)(&|$)", "jo")
Review comment:
Note that if the result is a table, get the first one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #4676: feat: support dash in args (#4519)
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #4676:
URL: https://github.com/apache/apisix/pull/4676#discussion_r681370460
##########
File path: t/core/ctx2.t
##########
@@ -136,3 +136,69 @@ qr/balancer_ip: 127.0.0.1|balancer_port: 1980/
--- grep_error_log_out
balancer_ip: 127.0.0.1
balancer_port: 1980
+
+
+
+=== TEST 5: support dash in the args
+--- config
+ location /t {
+ content_by_lua_block {
+ local core = require("apisix.core")
+ local ngx_ctx = ngx.ctx
+ local api_ctx = ngx_ctx.api_ctx
+ if api_ctx == nil then
+ api_ctx = core.tablepool.fetch("api_ctx", 0, 32)
Review comment:
It is not a good idea to set up api_ctx directly. We should let APISIX set it. See https://github.com/apache/apisix/pull/4676#discussion_r680302823
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] nanamikon commented on pull request #4676: feat: support dash in args (#4519)
Posted by GitBox <gi...@apache.org>.
nanamikon commented on pull request #4676:
URL: https://github.com/apache/apisix/pull/4676#issuecomment-891882039
Some conflicts should be solved, i will fix it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] nanamikon commented on a change in pull request #4676: feat: support dash in args (#4519)
Posted by GitBox <gi...@apache.org>.
nanamikon commented on a change in pull request #4676:
URL: https://github.com/apache/apisix/pull/4676#discussion_r679808294
##########
File path: apisix/core/ctx.lua
##########
@@ -159,7 +159,18 @@ do
val, err = cookie:get(sub_str(key, 8))
if err then
log.warn("failed to fetch cookie value by key: ",
- key, " error: ", err)
+ key, " error: ", err)
+ end
+ end
+
+ elseif core_str.has_prefix(key, "arg_") then
+ local arg_key = sub_str(key, 5)
+ local args = request.get_uri_args(ngx.ctx)[arg_key]
Review comment:
I add ngx.ctx, because of the following exception
```
Failed test 'TEST 7: support dash in the args(arg is missing) - pattern "[error]" should not match any line in error.log but matches line "2021/07/30 18:02:36 [error] 17776\#574167: *10 lua entry thread aborted: runtime error: /Users/jianzhou/workspace/apisix/apisix/core/request.lua:132: attempt to index local 'ctx' (a nil value)" (req 0)
# stack traceback:
# coroutine 0:
# /Users/jianzhou/workspace/apisix/apisix/core/request.lua: in function 'get_uri_args'
# /Users/jianzhou/workspace/apisix/apisix/core/ctx.lua:168: in function '__index'
# content_by_lua(nginx.conf:179):6: in main chunk, client: 127.0.0.1, server: localhost, request: "GET /t HTTP/1.1", host: "localhost"
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #4676: feat: support dash in args (#4519)
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #4676:
URL: https://github.com/apache/apisix/pull/4676#discussion_r681370460
##########
File path: t/core/ctx2.t
##########
@@ -136,3 +136,69 @@ qr/balancer_ip: 127.0.0.1|balancer_port: 1980/
--- grep_error_log_out
balancer_ip: 127.0.0.1
balancer_port: 1980
+
+
+
+=== TEST 5: support dash in the args
+--- config
+ location /t {
+ content_by_lua_block {
+ local core = require("apisix.core")
+ local ngx_ctx = ngx.ctx
+ local api_ctx = ngx_ctx.api_ctx
+ if api_ctx == nil then
+ api_ctx = core.tablepool.fetch("api_ctx", 0, 32)
Review comment:
It is not a good idea to set up api_ctx directly. We should let APISIX set it, so that we know the api_ctx is set correctly. See https://github.com/apache/apisix/pull/4676#discussion_r680302823
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] nanamikon commented on a change in pull request #4676: feat: support dash in args (#4519)
Posted by GitBox <gi...@apache.org>.
nanamikon commented on a change in pull request #4676:
URL: https://github.com/apache/apisix/pull/4676#discussion_r678399296
##########
File path: apisix/core/ctx.lua
##########
@@ -159,10 +160,22 @@ do
val, err = cookie:get(sub_str(key, 8))
if err then
log.warn("failed to fetch cookie value by key: ",
- key, " error: ", err)
+ key, " error: ", err)
end
end
+ elseif core_str.has_prefix(key, "arg_") then
+ local args = t.args
+ local arg_key = sub_str(key, 5)
+ local m, err = ngx_re_match(args, "(^|&)" .. arg_key .. "=([^&]*)(&|$)", "jo")
Review comment:
```
{
"vars": [
["uri_args_a-b","==","ab"]
]
}
```
If get_uri_args return a table, wil this work properly?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tzssangglass commented on a change in pull request #4676: feat: support dash in args (#4519)
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on a change in pull request #4676:
URL: https://github.com/apache/apisix/pull/4676#discussion_r681422348
##########
File path: apisix/core/ctx.lua
##########
@@ -159,10 +160,22 @@ do
val, err = cookie:get(sub_str(key, 8))
if err then
log.warn("failed to fetch cookie value by key: ",
- key, " error: ", err)
+ key, " error: ", err)
Review comment:
bad indent
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] nanamikon commented on a change in pull request #4676: feat: support dash in args (#4519)
Posted by GitBox <gi...@apache.org>.
nanamikon commented on a change in pull request #4676:
URL: https://github.com/apache/apisix/pull/4676#discussion_r679646809
##########
File path: t/core/ctx2.t
##########
@@ -136,3 +136,39 @@ qr/balancer_ip: 127.0.0.1|balancer_port: 1980/
--- grep_error_log_out
balancer_ip: 127.0.0.1
balancer_port: 1980
+
+
+
+=== TEST 5: support dash in the args
+--- config
+ location /t {
+ content_by_lua_block {
+ local core = require("apisix.core")
+ local ctx = {}
+ core.ctx.set_vars_meta(ctx)
+
+ ngx.say("uri_args_a-b: ", ctx.var["uri_args_a-b"])
+ }
+ }
+--- request
+GET /t?a-b=aaa
+--- response_body
+uri_args_a-b: aaa
+
+
+
+=== TEST 6: support dash in the args(Multi args with the same name, only fetch the first one)
Review comment:
ok
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] nanamikon commented on a change in pull request #4676: feat: support dash in args (#4519)
Posted by GitBox <gi...@apache.org>.
nanamikon commented on a change in pull request #4676:
URL: https://github.com/apache/apisix/pull/4676#discussion_r679808294
##########
File path: apisix/core/ctx.lua
##########
@@ -159,7 +159,18 @@ do
val, err = cookie:get(sub_str(key, 8))
if err then
log.warn("failed to fetch cookie value by key: ",
- key, " error: ", err)
+ key, " error: ", err)
+ end
+ end
+
+ elseif core_str.has_prefix(key, "arg_") then
+ local arg_key = sub_str(key, 5)
+ local args = request.get_uri_args(ngx.ctx)[arg_key]
Review comment:
I add ngx.ctx, because of the following exception
```
Failed test 'TEST 7: support dash in the args(arg is missing) - pattern "[error]" should not match any line in error.log but matches line "2021/07/30 18:02:36 [error] 17776\#574167: *10 lua entry thread aborted: runtime error: /Users/jianzhou/workspace/apisix/apisix/core/request.lua:132: attempt to index local 'ctx' (a nil value)" (req 0)
# stack traceback:
# coroutine 0:
# /Users/jianzhou/workspace/apisix/apisix/core/request.lua: in function 'get_uri_args'
# /Users/jianzhou/workspace/apisix/apisix/core/ctx.lua:168: in function '__index'
# content_by_lua(nginx.conf:179):6: in main chunk, client: 127.0.0.1, server: localhost, request: "GET /t HTTP/1.1", host: "localhost"
```
Should I modify the test case ?
Refer to request.t
```
local core = require("apisix.core")
local ngx_ctx = ngx.ctx
local api_ctx = ngx_ctx.api_ctx
if api_ctx == nil then
api_ctx = core.tablepool.fetch("api_ctx", 0, 32)
ngx_ctx.api_ctx = api_ctx
end
core.ctx.set_vars_meta(api_ctx)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] nanamikon commented on pull request #4676: feat: support dash in args (#4519)
Posted by GitBox <gi...@apache.org>.
nanamikon commented on pull request #4676:
URL: https://github.com/apache/apisix/pull/4676#issuecomment-893093663
> Err... We still need the "TEST 2: http header" but remove the arg part of it.
Sorry for misunderstanding， I will fix it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #4676: feat: support dash in args (#4519)
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #4676:
URL: https://github.com/apache/apisix/pull/4676#discussion_r679629897
##########
File path: apisix/core/ctx.lua
##########
@@ -164,16 +163,15 @@ do
end
end
- elseif core_str.has_prefix(key, "arg_") then
- local args = t.args
- local arg_key = sub_str(key, 5)
- local m, err = ngx_re_match(args, "(^|&)" .. arg_key .. "=([^&]*)(&|$)", "jo")
- if err then
- log.warn("failed to fetch arg value by key: ",
- key, " error: ", err)
- end
- if m then
- val = m[2]
+ elseif core_str.has_prefix(key, "uri_args_") then
Review comment:
Sorry for my mistake, I didn't make it clear
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] nanamikon commented on a change in pull request #4676: feat: support dash in args (#4519)
Posted by GitBox <gi...@apache.org>.
nanamikon commented on a change in pull request #4676:
URL: https://github.com/apache/apisix/pull/4676#discussion_r677053588
##########
File path: t/core/ctx.t
##########
@@ -1008,3 +1008,23 @@ GET /hello
hello world
--- error_log eval
qr/request log: \{"route_id":"1","route_name":"my_route","service_id":"1","service_name":"my_service"\}/
+
+
+
+=== TEST 36: support dash in the args
Review comment:
ok
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tokers commented on a change in pull request #4676: feat: support dash in args (#4519)
Posted by GitBox <gi...@apache.org>.
tokers commented on a change in pull request #4676:
URL: https://github.com/apache/apisix/pull/4676#discussion_r677052901
##########
File path: apisix/core/ctx.lua
##########
@@ -159,10 +160,22 @@ do
val, err = cookie:get(sub_str(key, 8))
if err then
log.warn("failed to fetch cookie value by key: ",
- key, " error: ", err)
+ key, " error: ", err)
end
end
+ elseif core_str.has_prefix(key, "arg_") then
+ local args = t.args
+ local arg_key = sub_str(key, 5);
Review comment:
```suggestion
local arg_key = sub_str(key, 5)
```
##########
File path: apisix/core/ctx.lua
##########
@@ -159,10 +160,22 @@ do
val, err = cookie:get(sub_str(key, 8))
if err then
log.warn("failed to fetch cookie value by key: ",
- key, " error: ", err)
+ key, " error: ", err)
end
end
+ elseif core_str.has_prefix(key, "arg_") then
+ local args = t.args
+ local arg_key = sub_str(key, 5);
+ local m, err = ngx_re_match(args, "(^|&)" .. arg_key .. "=([^&]*)(&|$)", "jo")
+ if err then
+ log.warn("failed to fetch arg value by key: ",
+ key, " error: ", err)
Review comment:
```suggestion
log.warn("failed to fetch arg value by key: ",
key, " error: ", err)
```
##########
File path: apisix/core/ctx.lua
##########
@@ -159,10 +160,22 @@ do
val, err = cookie:get(sub_str(key, 8))
if err then
log.warn("failed to fetch cookie value by key: ",
- key, " error: ", err)
+ key, " error: ", err)
Review comment:
Please revert this change, the original indent is good.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] nanamikon commented on a change in pull request #4676: feat: support dash in args (#4519)
Posted by GitBox <gi...@apache.org>.
nanamikon commented on a change in pull request #4676:
URL: https://github.com/apache/apisix/pull/4676#discussion_r679808294
##########
File path: apisix/core/ctx.lua
##########
@@ -159,7 +159,18 @@ do
val, err = cookie:get(sub_str(key, 8))
if err then
log.warn("failed to fetch cookie value by key: ",
- key, " error: ", err)
+ key, " error: ", err)
+ end
+ end
+
+ elseif core_str.has_prefix(key, "arg_") then
+ local arg_key = sub_str(key, 5)
+ local args = request.get_uri_args(ngx.ctx)[arg_key]
Review comment:
I add ngx.ctx, because the following exception
```
Failed test 'TEST 7: support dash in the args(arg is missing) - pattern "[error]" should not match any line in error.log but matches line "2021/07/30 18:02:36 [error] 17776\#574167: *10 lua entry thread aborted: runtime error: /Users/jianzhou/workspace/apisix/apisix/core/request.lua:132: attempt to index local 'ctx' (a nil value)" (req 0)
# stack traceback:
# coroutine 0:
# /Users/jianzhou/workspace/apisix/apisix/core/request.lua: in function 'get_uri_args'
# /Users/jianzhou/workspace/apisix/apisix/core/ctx.lua:168: in function '__index'
# content_by_lua(nginx.conf:179):6: in main chunk, client: 127.0.0.1, server: localhost, request: "GET /t HTTP/1.1", host: "localhost"
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on pull request #4676: feat: support dash in args (#4519)
Posted by GitBox <gi...@apache.org>.
spacewander commented on pull request #4676:
URL: https://github.com/apache/apisix/pull/4676#issuecomment-892569214
Err... We still need the "TEST 2: http header" but remove the arg part of it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] nanamikon commented on pull request #4676: feat: support dash in args (#4519)
Posted by GitBox <gi...@apache.org>.
nanamikon commented on pull request #4676:
URL: https://github.com/apache/apisix/pull/4676#issuecomment-893093663
> Err... We still need the "TEST 2: http header" but remove the arg part of it.
Sorry for misunderstanding， I will fix it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #4676: feat: support dash in args (#4519)
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #4676:
URL: https://github.com/apache/apisix/pull/4676#discussion_r677899476
##########
File path: apisix/core/ctx.lua
##########
@@ -159,10 +160,22 @@ do
val, err = cookie:get(sub_str(key, 8))
if err then
log.warn("failed to fetch cookie value by key: ",
- key, " error: ", err)
+ key, " error: ", err)
end
end
+ elseif core_str.has_prefix(key, "arg_") then
+ local args = t.args
+ local arg_key = sub_str(key, 5)
+ local m, err = ngx_re_match(args, "(^|&)" .. arg_key .. "=([^&]*)(&|$)", "jo")
Review comment:
We can use `core.request.get_uri_args`, which is more clear?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] nanamikon commented on pull request #4676: feat: support dash in args (#4519)
Posted by GitBox <gi...@apache.org>.
nanamikon commented on pull request #4676:
URL: https://github.com/apache/apisix/pull/4676#issuecomment-892320857
All tests pass now
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] nanamikon commented on a change in pull request #4676: feat: support dash in args (#4519)
Posted by GitBox <gi...@apache.org>.
nanamikon commented on a change in pull request #4676:
URL: https://github.com/apache/apisix/pull/4676#discussion_r679008963
##########
File path: apisix/core/ctx.lua
##########
@@ -159,10 +160,22 @@ do
val, err = cookie:get(sub_str(key, 8))
if err then
log.warn("failed to fetch cookie value by key: ",
- key, " error: ", err)
+ key, " error: ", err)
end
end
+ elseif core_str.has_prefix(key, "arg_") then
+ local args = t.args
+ local arg_key = sub_str(key, 5)
+ local m, err = ngx_re_match(args, "(^|&)" .. arg_key .. "=([^&]*)(&|$)", "jo")
Review comment:
get it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #4676: feat: support dash in args (#4519)
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #4676:
URL: https://github.com/apache/apisix/pull/4676#discussion_r677952502
##########
File path: apisix/core/ctx.lua
##########
@@ -159,10 +160,22 @@ do
val, err = cookie:get(sub_str(key, 8))
if err then
log.warn("failed to fetch cookie value by key: ",
- key, " error: ", err)
+ key, " error: ", err)
end
end
+ elseif core_str.has_prefix(key, "arg_") then
+ local args = t.args
+ local arg_key = sub_str(key, 5)
+ local m, err = ngx_re_match(args, "(^|&)" .. arg_key .. "=([^&]*)(&|$)", "jo")
Review comment:
Something like `core.request.get_uri_args()["a-b"]`, note that if the result is a table, get the first one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on pull request #4676: feat: support dash in args (#4519)
Posted by GitBox <gi...@apache.org>.
spacewander commented on pull request #4676:
URL: https://github.com/apache/apisix/pull/4676#issuecomment-891701652
Please merge master to make CI pass.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #4676: feat: support dash in args (#4519)
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #4676:
URL: https://github.com/apache/apisix/pull/4676#discussion_r677048124
##########
File path: t/core/ctx.t
##########
@@ -1008,3 +1008,23 @@ GET /hello
hello world
--- error_log eval
qr/request log: \{"route_id":"1","route_name":"my_route","service_id":"1","service_name":"my_service"\}/
+
+
+
+=== TEST 36: support dash in the args
Review comment:
Could you add the test in t/core/ctx2.t? Note that we already set up request & no_error_log there:
https://github.com/apache/apisix/blob/e3e018315311e0ca201f17579f9106eb88a834d1/t/core/ctx2.t#L23-L37
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] nanamikon commented on pull request #4676: feat: support dash in args (#4519)
Posted by GitBox <gi...@apache.org>.
nanamikon commented on pull request #4676:
URL: https://github.com/apache/apisix/pull/4676#issuecomment-891927410
> Please merge master to make CI pass.
CI fail refering to error log
```
2021/08/03 22:55:24 [error] 21865#948770: *10 lua entry thread aborted: runtime error: /Users/jianzhou/workspace/apisix/apisix/core/request.lua:132: attempt to index local 'ctx' (a nil value)
stack traceback:
coroutine 0:
/Users/jianzhou/workspace/apisix/apisix/core/request.lua: in function 'get_uri_args'
/Users/jianzhou/workspace/apisix/apisix/core/ctx.lua:169: in function '__index'
/Users/jianzhou/workspace/apisix/apisix/admin/init.lua:71: in function 'check_token'
/Users/jianzhou/workspace/apisix/apisix/admin/init.lua:123: in function 'handler'
...workspace/apisix//deps/share/lua/5.1/resty/radixtree.lua:722: in function 'dispatch'
/Users/jianzhou/workspace/apisix/apisix/init.lua:765: in function 'http_admin'
content_by_lua(nginx.conf:222):2: in main chunk, client: 127.0.0.1, server: , request: "GET /apisix/admin/routes HTTP/1.1", host: "127.0.0.1:9180"
```
I think it should pass api_ctx to ngx.ctx in the /admin/init.lua
```
local function run()
local api_ctx = {}
core.ctx.set_vars_meta(api_ctx)
local ok, err = check_token(api_ctx)
```
Do I need to fit this in this pr?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] nanamikon commented on a change in pull request #4676: feat: support dash in args (#4519)
Posted by GitBox <gi...@apache.org>.
nanamikon commented on a change in pull request #4676:
URL: https://github.com/apache/apisix/pull/4676#discussion_r679646865
##########
File path: apisix/core/ctx.lua
##########
@@ -164,16 +163,15 @@ do
end
end
- elseif core_str.has_prefix(key, "arg_") then
- local args = t.args
- local arg_key = sub_str(key, 5)
- local m, err = ngx_re_match(args, "(^|&)" .. arg_key .. "=([^&]*)(&|$)", "jo")
- if err then
- log.warn("failed to fetch arg value by key: ",
- key, " error: ", err)
- end
- if m then
- val = m[2]
+ elseif core_str.has_prefix(key, "uri_args_") then
Review comment:
It's ok
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on pull request #4676: feat: support dash in args (#4519)
Posted by GitBox <gi...@apache.org>.
spacewander commented on pull request #4676:
URL: https://github.com/apache/apisix/pull/4676#issuecomment-892569214
Err... We still need the "TEST 2: http header" but remove the arg part of it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #4676: feat: support dash in args (#4519)
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #4676:
URL: https://github.com/apache/apisix/pull/4676#discussion_r679715749
##########
File path: apisix/core/ctx.lua
##########
@@ -159,7 +159,18 @@ do
val, err = cookie:get(sub_str(key, 8))
if err then
log.warn("failed to fetch cookie value by key: ",
- key, " error: ", err)
+ key, " error: ", err)
+ end
+ end
+
+ elseif core_str.has_prefix(key, "arg_") then
+ local arg_key = sub_str(key, 5)
+ local args = request.get_uri_args(ngx.ctx)[arg_key]
Review comment:
We don't need to pass `ngx.ctx`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] nanamikon commented on pull request #4676: feat: support dash in args (#4519)
Posted by GitBox <gi...@apache.org>.
nanamikon commented on pull request #4676:
URL: https://github.com/apache/apisix/pull/4676#issuecomment-892343554
Get it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander merged pull request #4676: feat: support dash in args (#4519)
Posted by GitBox <gi...@apache.org>.
spacewander merged pull request #4676:
URL: https://github.com/apache/apisix/pull/4676
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] nanamikon removed a comment on pull request #4676: feat: support dash in args (#4519)
Posted by GitBox <gi...@apache.org>.
nanamikon removed a comment on pull request #4676:
URL: https://github.com/apache/apisix/pull/4676#issuecomment-891900248
> Please merge master to make CI pass.
How to deal with these ci test error? It seems not relate to my pr
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tzssangglass commented on a change in pull request #4676: feat: support dash in args (#4519)
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on a change in pull request #4676:
URL: https://github.com/apache/apisix/pull/4676#discussion_r683073663
##########
File path: apisix/core/ctx.lua
##########
@@ -159,10 +160,22 @@ do
val, err = cookie:get(sub_str(key, 8))
if err then
log.warn("failed to fetch cookie value by key: ",
- key, " error: ", err)
+ key, " error: ", err)
Review comment:
```suggestion
key, " error: ", err)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] nanamikon commented on a change in pull request #4676: feat: support dash in args (#4519)
Posted by GitBox <gi...@apache.org>.
nanamikon commented on a change in pull request #4676:
URL: https://github.com/apache/apisix/pull/4676#discussion_r679808294
##########
File path: apisix/core/ctx.lua
##########
@@ -159,7 +159,18 @@ do
val, err = cookie:get(sub_str(key, 8))
if err then
log.warn("failed to fetch cookie value by key: ",
- key, " error: ", err)
+ key, " error: ", err)
+ end
+ end
+
+ elseif core_str.has_prefix(key, "arg_") then
+ local arg_key = sub_str(key, 5)
+ local args = request.get_uri_args(ngx.ctx)[arg_key]
Review comment:
I add ngx.ctx, because of the following exception
```
Failed test 'TEST 7: support dash in the args(arg is missing) - pattern "[error]" should not match any line in error.log but matches line "2021/07/30 18:02:36 [error] 17776\#574167: *10 lua entry thread aborted: runtime error: /Users/jianzhou/workspace/apisix/apisix/core/request.lua:132: attempt to index local 'ctx' (a nil value)" (req 0)
# stack traceback:
# coroutine 0:
# /Users/jianzhou/workspace/apisix/apisix/core/request.lua: in function 'get_uri_args'
# /Users/jianzhou/workspace/apisix/apisix/core/ctx.lua:168: in function '__index'
# content_by_lua(nginx.conf:179):6: in main chunk, client: 127.0.0.1, server: localhost, request: "GET /t HTTP/1.1", host: "localhost"
```
Should I modify the test to let it work?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #4676: feat: support dash in args (#4519)
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #4676:
URL: https://github.com/apache/apisix/pull/4676#discussion_r679630143
##########
File path: t/core/ctx2.t
##########
@@ -136,3 +136,39 @@ qr/balancer_ip: 127.0.0.1|balancer_port: 1980/
--- grep_error_log_out
balancer_ip: 127.0.0.1
balancer_port: 1980
+
+
+
+=== TEST 5: support dash in the args
+--- config
+ location /t {
+ content_by_lua_block {
+ local core = require("apisix.core")
+ local ctx = {}
+ core.ctx.set_vars_meta(ctx)
+
+ ngx.say("uri_args_a-b: ", ctx.var["uri_args_a-b"])
+ }
+ }
+--- request
+GET /t?a-b=aaa
+--- response_body
+uri_args_a-b: aaa
+
+
+
+=== TEST 6: support dash in the args(Multi args with the same name, only fetch the first one)
Review comment:
Let's add a test that arg is missing
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] nanamikon commented on a change in pull request #4676: feat: support dash in args (#4519)
Posted by GitBox <gi...@apache.org>.
nanamikon commented on a change in pull request #4676:
URL: https://github.com/apache/apisix/pull/4676#discussion_r677937756
##########
File path: apisix/core/ctx.lua
##########
@@ -159,10 +160,22 @@ do
val, err = cookie:get(sub_str(key, 8))
if err then
log.warn("failed to fetch cookie value by key: ",
- key, " error: ", err)
+ key, " error: ", err)
end
end
+ elseif core_str.has_prefix(key, "arg_") then
+ local args = t.args
+ local arg_key = sub_str(key, 5)
+ local m, err = ngx_re_match(args, "(^|&)" .. arg_key .. "=([^&]*)(&|$)", "jo")
Review comment:
Do you mean?
```
args : a-b=ab
core.request.get_uri_args_a-b : ab
```
if *uri* is for making args different from args in nginx var， I think the following example is clear enough?
```
args : a-b=ab
core.request.get_args_a-b : ab
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tzssangglass commented on a change in pull request #4676: feat: support dash in args (#4519)
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on a change in pull request #4676:
URL: https://github.com/apache/apisix/pull/4676#discussion_r683073663
##########
File path: apisix/core/ctx.lua
##########
@@ -159,10 +160,22 @@ do
val, err = cookie:get(sub_str(key, 8))
if err then
log.warn("failed to fetch cookie value by key: ",
- key, " error: ", err)
+ key, " error: ", err)
Review comment:
```suggestion
key, " error: ", err)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #4676: feat: support dash in args (#4519)
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #4676:
URL: https://github.com/apache/apisix/pull/4676#discussion_r680302823
##########
File path: apisix/core/ctx.lua
##########
@@ -159,7 +159,18 @@ do
val, err = cookie:get(sub_str(key, 8))
if err then
log.warn("failed to fetch cookie value by key: ",
- key, " error: ", err)
+ key, " error: ", err)
+ end
+ end
+
+ elseif core_str.has_prefix(key, "arg_") then
+ local arg_key = sub_str(key, 5)
+ local args = request.get_uri_args(ngx.ctx)[arg_key]
Review comment:
The api_ctx is set by the other part.
You can refer to this test: https://github.com/apache/apisix/pull/4723/files
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] nanamikon commented on pull request #4676: feat: support dash in args (#4519)
Posted by GitBox <gi...@apache.org>.
nanamikon commented on pull request #4676:
URL: https://github.com/apache/apisix/pull/4676#issuecomment-891900248
> Please merge master to make CI pass.
How to deal with these ci test error? It seems not relate to my pr
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] nanamikon commented on a change in pull request #4676: feat: support dash in args (#4519)
Posted by GitBox <gi...@apache.org>.
nanamikon commented on a change in pull request #4676:
URL: https://github.com/apache/apisix/pull/4676#discussion_r681367857
##########
File path: apisix/core/ctx.lua
##########
@@ -159,7 +159,18 @@ do
val, err = cookie:get(sub_str(key, 8))
if err then
log.warn("failed to fetch cookie value by key: ",
- key, " error: ", err)
+ key, " error: ", err)
+ end
+ end
+
+ elseif core_str.has_prefix(key, "arg_") then
+ local arg_key = sub_str(key, 5)
+ local args = request.get_uri_args(ngx.ctx)[arg_key]
Review comment:
Get it, I find it inited in the init.lua
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #4676: feat: support dash in args (#4519)
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #4676:
URL: https://github.com/apache/apisix/pull/4676#discussion_r679629577
##########
File path: apisix/core/ctx.lua
##########
@@ -164,16 +163,15 @@ do
end
end
- elseif core_str.has_prefix(key, "arg_") then
- local args = t.args
- local arg_key = sub_str(key, 5)
- local m, err = ngx_re_match(args, "(^|&)" .. arg_key .. "=([^&]*)(&|$)", "jo")
- if err then
- log.warn("failed to fetch arg value by key: ",
- key, " error: ", err)
- end
- if m then
- val = m[2]
+ elseif core_str.has_prefix(key, "uri_args_") then
+ local arg_key = sub_str(key, 10)
+ local args = ngx.req.get_uri_args()[arg_key]
Review comment:
Let's use `core.request.get_uri_args`
##########
File path: apisix/core/ctx.lua
##########
@@ -164,16 +163,15 @@ do
end
end
- elseif core_str.has_prefix(key, "arg_") then
- local args = t.args
- local arg_key = sub_str(key, 5)
- local m, err = ngx_re_match(args, "(^|&)" .. arg_key .. "=([^&]*)(&|$)", "jo")
- if err then
- log.warn("failed to fetch arg value by key: ",
- key, " error: ", err)
- end
- if m then
- val = m[2]
+ elseif core_str.has_prefix(key, "uri_args_") then
Review comment:
Err... Let's use the old name "arg_".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on pull request #4676: feat: support dash in args (#4519)
Posted by GitBox <gi...@apache.org>.
spacewander commented on pull request #4676:
URL: https://github.com/apache/apisix/pull/4676#issuecomment-892337187
We can remove the 'arg' part from 'TEST 2: http header + arg'
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org