fix: preserve uri args if path has not been modified#13080
fix: preserve uri args if path has not been modified#13080shreemaan-abhishek merged 3 commits intoapache:masterfrom
Conversation
Signed-off-by: Abhishek Choudhary <shreemaan.abhishek@gmail.com>
dosubot
bot
added
size:M
There was a problem hiding this comment.
Pull request overview
Fixes upstream URI construction for ext-plugin rewrites so that when only the path is rewritten, the original request query string is still forwarded upstream.
Changes:
- Update ext-plugin rewrite handling to always append the (current) query string when a rewritten path is present.
- Add an ext-plugin test mode to rewrite only the path (no args rewrite).
- Add regression tests covering “path-only rewrite preserves original query args”.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| t/plugin/ext-plugin/http-req-call.t | Adds regression tests for preserving query args when only the path is rewritten. |
| t/lib/ext-plugin.lua | Adds a new test-case branch that rewrites only the path via the ext-plugin response. |
| apisix/plugins/ext-plugin/init.lua | Adjusts upstream URI construction so query args are preserved even when args weren’t explicitly rewritten. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
apisix/plugins/ext-plugin/init.lua
Outdated
| var.upstream_uri = path .. '?' .. var.args | ||
| end | ||
| if path then | ||
| var.upstream_uri = path .. '?' .. var.args |
There was a problem hiding this 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.
| var.upstream_uri = path .. '?' .. var.args | |
| 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 |
| === TEST 30: hit (original query args should be preserved when only path is rewritten) | ||
| --- request | ||
| GET /hello?c=foo&d=bar |
There was a problem hiding this 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
Outdated
| local action = http_req_call_rewrite.End(builder) | ||
| build_action(action, http_req_call_action.Rewrite) | ||
|
|
||
| elseif case.rewrite_path_only == true then |
There was a problem hiding this 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.
| elseif case.rewrite_path_only == true then | |
| elseif case.rewrite_path_only then |
apisix/plugins/ext-plugin/init.lua
Outdated
| if path then | ||
| var.upstream_uri = path .. '?' .. var.args | ||
| if path then | ||
| if var.args and var.args ~= "" and not core.string.find(path, "?") then |
There was a problem hiding this comment.
can we use this variable var.is_args?
https://nginx.org/en/docs/http/ngx_http_core_module.html#var_is_args
Signed-off-by: Abhishek Choudhary <shreemaan.abhishek@gmail.com>
Description
when the path is rewritten but no args are modified, we still need to append the original query string to upstream_uri.
Fixes #12240
Checklist