Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Uri parameters in vars expression #142

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

boekkooi-lengoo
Copy link
Contributor

This PR is a spin-off from #141 as the implementation differs a lot but causes the same effect.

This will allow us to use the matched params as part of the vars later.
This adds `uri_param_<name>` allowing vars and filters access to the parameters of a uri.
By doing this we are able to improve validation of routes to specific params.
As match_route_opts now handles both the opts and params we can set the matched information in match_route_opts to avoid some duplicate code.
It seems `setmetatable` is very slow and was destroying the benchmark. For this reason we now use `clone_tab` and add the uri matches.
No need to capture this data.
Copy link
Contributor

@membphis membphis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI failed of master branch, we need to fix it first.

I have created an issue: #143

lib/resty/radixtree.lua Outdated Show resolved Hide resolved
There is no need to return `nil` as this is the default so let's avoid adding it.
@membphis
Copy link
Contributor

@boekkooi-lengoo I have helped you to rerun the CI, the test cases had passed. You can continue this PR

@boekkooi-lengoo
Copy link
Contributor Author

Hey @membphis

Could you maybe review the code and let me know if it requires some changes before I write the docs?

Thanks in advance!

@membphis
Copy link
Contributor

will do this tomorrow

@membphis
Copy link
Contributor

sorry for my late reply, it take me more time to read and test your code, they are all fine to me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants