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

Support request.query decoding #126

Merged
merged 8 commits into from
Nov 7, 2024
Merged

Support request.query decoding #126

merged 8 commits into from
Nov 7, 2024

Conversation

alexsnaps
Copy link
Member

@alexsnaps alexsnaps commented Oct 29, 2024

See test, but this effectively works like this, given the query string param1=%F0%9F%91%BE%20&param2=Exterminate%21&%F0%9F%91%BE=123&%F0%9F%91%BE=456

queryMap(request.query)['param1'] == '👾 ' &&
queryMap(request.query)['param2'] == 'Exterminate!' &&
queryMap(request.query)['👾'][0] == '123' &&
queryMap(request.query)['👾'][1] == '456'

i.e.:

  • Doesn't trim
  • decodes all utf-8
  • supports lists of values

It is exposed as an "extension" that's only enabled via Expression::new_extended, but mostly thru Predicate::route_rule that's now only used in the config, when compiling RouteRuleConditions

Copy link
Contributor

@eguzki eguzki left a comment

Choose a reason for hiding this comment

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

LGTM

Add some doc and ready to be merged

src/data/cel.rs Outdated Show resolved Hide resolved
src/data/cel.rs Outdated Show resolved Hide resolved
@alexsnaps
Copy link
Member Author

alexsnaps commented Oct 30, 2024

@guicassolato so this works for your use-case(s)?
Also, that last test is somewhat sad... wdy'all think?

Copy link
Contributor

@didierofrivia didierofrivia left a comment

Choose a reason for hiding this comment

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

👾

@alexsnaps alexsnaps requested a review from eguzki November 4, 2024 10:33
@alexsnaps alexsnaps force-pushed the query_string branch 2 times, most recently from 0b63a97 to c9ae52a Compare November 5, 2024 13:19
@alexsnaps
Copy link
Member Author

@eguzki is this ok? Or are you still expecting more changes?

@alexsnaps
Copy link
Member Author

So reading this:

// If a query param is repeated in an HTTP request, the behavior is
// purposely left undefined, since different data planes have different
// capabilities. However, it is *recommended* that implementations should
// match against the first value of the param if the data plane supports it,
// as this behavior is expected in other load balancing contexts outside of
// the Gateway API.
//
// Users SHOULD NOT route traffic based on repeated query params to guard
// themselves against potential differences in the implementations.

Wondering if we shouldn't always have the first param in our Map and drop the List altogether...
I have a PR getting ready that would:

fmt.Sprintf("decodeQueryString(request.query)['%s'] == '%s'", queryParam.Name, queryParam.Value)

@guicassolato do you have an opinion?

@alexsnaps
Copy link
Member Author

alexsnaps commented Nov 5, 2024

Alternate proposal, that I'll implement here right now, pass an optional 2nd bool param, that would dictate whether to return a List when repeated params are present, or only the first one (includeRepeatedParams: bool kinda thing).

See this PR on the Kuadrant Operator side

@alexsnaps alexsnaps self-assigned this Nov 6, 2024
@eguzki
Copy link
Contributor

eguzki commented Nov 7, 2024

In description it says decodeQueryString but in the implementation I see queryMap.

@eguzki eguzki merged commit da74bcd into main Nov 7, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants