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

Add option for whether semicolon is treated as normal character or not #5150

Merged

Conversation

PragmaTwice
Copy link
Contributor

@PragmaTwice PragmaTwice commented Mar 12, 2024

Motivation:

Close #4588.

This PR adds an option uriQuerySemicolonIsNormalChar to HttpServerOptions to support treating ; a normal character instead of query parameter seperator, since it has already been supported in netty QueryStringDecoder.

And the default behavior is still to treat ; as a seperator to ensure compatibility.

References:

Conformance:

You should have signed the Eclipse Contributor Agreement as explained in https://github.com/eclipse/vert.x/blob/master/CONTRIBUTING.md

Please also make sure you adhere to the code style guidelines: https://github.com/vert-x3/wiki/wiki/Vert.x-code-style-guidelines

@vietj
Copy link
Member

vietj commented Mar 12, 2024

I don't think we are going to make this an option as it is very specific to the API and not related to the behavior of the HTTP server itself

@vietj
Copy link
Member

vietj commented Mar 12, 2024

Instead it should be a method parameter of Http1xServerRequest#params() method

Copy link
Member

@vietj vietj left a comment

Choose a reason for hiding this comment

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

Use a method parameter instead of an HTTP server option

@PragmaTwice
Copy link
Contributor Author

PragmaTwice commented Mar 13, 2024

Use a method parameter instead of an HTTP server option

Done according to your suggestions : )

Feel free to review again and comment. @vietj

@PragmaTwice PragmaTwice requested a review from vietj March 14, 2024 12:06
Copy link
Member

@vietj vietj left a comment

Choose a reason for hiding this comment

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

since the created multimap is cached, we should also cache the delimiter and check the delimiter is the same when accessing again the multimap because this might be called multiple times with different values, when the delimiter is not the same then we should compute it again (and perhaps cache it).

@PragmaTwice
Copy link
Contributor Author

since the created multimap is cached, we should also cache the delimiter and check the delimiter is the same when accessing again the multimap because this might be called multiple times with different values, when the delimiter is not the same then we should compute it again (and perhaps cache it).

Thank you ! I see.

I've added a new cached property semicolonIsNormalCharInParams and use it to check if the cached params need to be refreshed.

Please review it again : )

@PragmaTwice
Copy link
Contributor Author

PragmaTwice commented Mar 28, 2024

Fixed according to your suggestions : )

Feel free to review again and comment. cc @vietj

@PragmaTwice PragmaTwice requested a review from vietj March 29, 2024 09:18
Copy link
Member

@vietj vietj left a comment

Choose a reason for hiding this comment

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

This test only the new feature with HTTP/2, a new test should be added in HttpTest so it will be executed for for HTTP/2 and HTTP/1.

@PragmaTwice
Copy link
Contributor Author

Fixed according to your suggestions : )

Feel free to review again and comment. cc @vietj

@PragmaTwice
Copy link
Contributor Author

Fixed according to your suggestions : )

Feel free to review again and comment. cc @vietj

Copy link
Member

@vietj vietj left a comment

Choose a reason for hiding this comment

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

Can you squash everything in a single commit to facilitate back-port to 4.x ?

Close eclipse-vertx#4588.

This PR adds an option uriQuerySemicolonIsNormalChar to HttpServerOptions to support treating ; a normal character instead of query parameter seperator, since it has already been supported in netty QueryStringDecoder.

And the default behavior is still to treat ; as a seperator to ensure compatibility.
@PragmaTwice PragmaTwice force-pushed the add-semicolon-normal-char-option branch from 0620dd8 to 4a0d5c4 Compare April 8, 2024 16:17
@PragmaTwice
Copy link
Contributor Author

Can you squash everything in a single commit to facilitate back-port to 4.x ?

Done. Please check.

@vietj
Copy link
Member

vietj commented Apr 8, 2024

thanks I'll merge that soon and backport

@vietj vietj merged commit a1444c5 into eclipse-vertx:master Apr 9, 2024
7 checks passed
@vietj vietj added this to the 5.0.0 milestone Apr 9, 2024
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.

Allow semicolon to be treated as normal character in query params
2 participants