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

[WIP/PoC] feat(router): add http query param match #11072

Closed
wants to merge 1 commit into from

Conversation

randmonkey
Copy link
Contributor

Summary

This is the PoC of adding http.queries.* to match query parameters in HTTP requests.

Checklist

Full changelog

  • [Implement ...]

Issue reference

Fix #[issue number]

@randmonkey randmonkey marked this pull request as draft June 15, 2023 06:45
@randmonkey randmonkey force-pushed the poc/try_add_query_param_match branch from 186285b to b973cb5 Compare June 15, 2023 08:05
@randmonkey randmonkey changed the title poc(expression router) add http query param match [WIP/PoC] feat(router) add http query param match Jun 15, 2023
@randmonkey randmonkey changed the title [WIP/PoC] feat(router) add http query param match [WIP/PoC] feat(router): add http query param match Jun 15, 2023
@chronolaw
Copy link
Contributor

Technically it is possible, but we should consider the compatible issue of flavor traditional.

@cs1hsb
Copy link

cs1hsb commented Jun 20, 2023

+1 This looks very interesting - looking forward to it :)

It feels like the new router expressions are to provide more powerful matching, so maybe it is acceptable for them to go beyond traditional capabilities..? :)

@dndx
Copy link
Member

dndx commented Jul 6, 2023

Technically it is possible, but we should consider the compatible issue of flavor traditional.

@chronolaw Could you clarify why it causes compatibility issue with traditional? traditional is feature frozen at this point and we do not plan on porting this feature to it.

@chronolaw
Copy link
Contributor

Technically it is possible, but we should consider the compatible issue of flavor traditional.

@chronolaw Could you clarify why it causes compatibility issue with traditional? traditional is feature frozen at this point and we do not plan on porting this feature to it.

We should add a new field in the route schema, which is shared by traditional and traditional_compatible.

If you are right, then we can only implement it in traditional_compatible

@dndx
Copy link
Member

dndx commented Jul 7, 2023

This is a expressions only feature, we should not add it to traditional or traditional_compatible, they are feature frozen.

cc @randmonkey

@chronolaw
Copy link
Contributor

The code looks nice. After #11071 is merged we will work on it.

@chronolaw
Copy link
Contributor

chronolaw commented Aug 3, 2023

Replaced by #11348

@chronolaw chronolaw closed this Aug 3, 2023
@chronolaw chronolaw deleted the poc/try_add_query_param_match branch August 3, 2023 03:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants