feat: Provide uri_arg_ when radixtree_uri_with_parameter#10645
Conversation
Add the prefix `uri_arg_` to the APISIX `ctx.var` allowing a APISIX configured to use the `radixtree_uri_with_parameter` router to access `opts.matched`. This allows the usage of the matches inside for example the `proxy_rewrite` plugin. Also see https://github.com/api7/lua-resty-radixtree/#parameters-in-path
15d52e2 to
cd13465
Compare
|
@boekkooi-lengoo good catch 👍 |
|
@moonming I'm fine adding more just not sure which ones. Any suggestions what you think is missing? |
|
@boekkooi-lengoo please make the ci pass |
@boekkooi-lengoo you can add a test case where the uri parameter does not exist and see the failure situation. Others are LGTM |
Executed `find t -name '*.t' -exec ./utils/reindex '{}' \;` which should resolve the linting error.
To avoid unexpected behaviour a few extra test case have been added that ensure the following. - `uri_arg_path` is actually dynamic and not hard coded to `hello` - When `uri_arg_path` is pointing to an unknown route it's handled - When a `uri_arg_<arg>` is not configured a warning is shown and an empty value (nil) is returned.
|
Good day @moonming and @monkeyDluffy6017, Thanks for the review! Please let me know if I missed anything 😄 |
great work. looks good to me👍 |
| end | ||
| end | ||
|
|
||
| elseif core_str.has_prefix(key, "uri_arg_") then |
There was a problem hiding this comment.
uri_arg always means URL query arguments, maybe you should use uri_para_ instead?
There was a problem hiding this comment.
How about uri_param_?
juststillthinking
added
the
wait for update
Naming is hard and as it's called parameter in the radix router we follow it's naming convention.
As there is no actual error (except one by the user) we avoid logging it.
It seems we don't need to check sanity so we remove it.
e13e826
Ensure people know that `uri_param_` should only be used when `radixtree_uri_with_parameter` is in use.
|
Hey @monkeyDluffy6017 Can you please give the PR another look. I think all comments where addressed. Thanks in advance. |
|
please make the ci pass |
Seems linting warning are errors in the ci ``` apisix/core/ctx.lua:278:101: line is too long (115 > 100) ```
|
Hey @monkeyDluffy6017 I fixed the linting issue but I can't reproduce the |
|
please merge the master |
|
@monkeyDluffy6017 Created merge commit 45ee4a2 as requested |
juststillthinking
added
approved
and removed
wait for update
6 participants
Description
Add the prefix
uri_arg_to the APISIXctx.varallowing a APISIX configured to use theradixtree_uri_with_parameterrouter to accessopts.matched. This allows the usage of the matches inside for example theproxy_rewriteplugin.Also see https://github.com/api7/lua-resty-radixtree/#parameters-in-path
Fixes #10257
Checklist