Copilot commented on code in PR #13080:
URL: https://github.com/apache/apisix/pull/13080#discussion_r2904234947
##########
apisix/plugins/ext-plugin/init.lua:
##########
@@ -706,10 +706,10 @@ local rpc_handlers = {
end
core.request.set_uri_args(ctx, args)
+ end
- if path then
- var.upstream_uri = path .. '?' .. var.args
- end
+ if path then
+ var.upstream_uri = path .. '?' .. var.args
Review Comment:
`var.upstream_uri = path .. '?' .. var.args` will produce a trailing `?`
when `var.args` is empty, and can also produce malformed URIs if `path` ever
contains a `?` already. Consider building `upstream_uri` as `path` first, and
only appending `?` + args when `var.args` is non-empty (and, if applicable in
this codepath, ensure `path` does not already contain a query string before
concatenation). This preserves the intended fix while avoiding generating
invalid/odd upstream URIs.
```suggestion
if var.args and var.args ~= "" and not string.find(path,
"?", 1, true) then
var.upstream_uri = path .. "?" .. var.args
else
var.upstream_uri = path
end
```
##########
t/plugin/ext-plugin/http-req-call.t:
##########
@@ -807,3 +807,60 @@ cat
}
--- response_body
abc
+
+
+
+=== TEST 29: rewrite path only (preserve original query args)
+--- config
+ location /t {
+ content_by_lua_block {
+ local json = require("toolkit.json")
+ local t = require("lib.test_admin")
+
+ local code, message, res = t.test('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "uri": "/hello",
+ "plugins": {
+ "ext-plugin-pre-req": {
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1980": 1
+ },
+ "type": "roundrobin"
+ }
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ ngx.say(message)
+ return
+ end
+
+ ngx.say(message)
+ }
+ }
+--- response_body
+passed
+
+
+
+=== TEST 30: hit (original query args should be preserved when only path is
rewritten)
+--- request
+GET /hello?c=foo&d=bar
Review Comment:
The new behavior is covered for non-empty query strings, but there isn’t a
regression test for the “no query string” case. Adding a test like `GET /hello`
(no args) with `rewrite_path_only = true` would help ensure the upstream URI
doesn’t end up with a trailing `?` and remains exactly the rewritten path.
##########
t/lib/ext-plugin.lua:
##########
@@ -399,6 +399,13 @@ function _M.go(case)
local action = http_req_call_rewrite.End(builder)
build_action(action, http_req_call_action.Rewrite)
+ elseif case.rewrite_path_only == true then
Review Comment:
Minor style/readability: in Lua this is typically written as `elseif
case.rewrite_path_only then` since the field is already boolean-ish. This keeps
consistency with common Lua conventions and reduces noise in conditions.
```suggestion
elseif case.rewrite_path_only then
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
