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.

Sign up for GitHub

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

Jump to bottom

[UNDERTOW-2312] multibytes language in URL request to http/https are … #1516

Merged
merged 3 commits into from
Aug 22, 2024
Merged

[UNDERTOW-2312] multibytes language in URL request to http/https are … #1516

merged 3 commits into from
Aug 22, 2024

Conversation

baranowb
Copy link
Contributor

…broken in EAP access log.
ISsue: https://issues.redhat.com/browse/UNDERTOW-2312

I need to check if there are TCs for access log. Im pushing this just for review ATM.

@fl4via fl4via added the waiting PR update Awaiting PR update(s) from contributor before merging label Oct 7, 2023
@fl4via
Copy link
Member

fl4via commented Oct 7, 2023

I'll update this PR myself as I have a test that is ready for HTTP and now I'm trying to come up with a https one.

@fl4via fl4via added bug fix Contains bug fix(es) next release This PR will be merged before next release or has already been merged (for payload double check) failed CI Introduced new regession(s) during CI check under verification Currently being verified (running tests, reviewing) before posting a review to contributor and removed waiting PR update Awaiting PR update(s) from contributor before merging labels Oct 9, 2023
@fl4via
Copy link
Member

fl4via commented Oct 10, 2023

Right now I'm working on making the test work with Http2 upgrade. It seems I solved the AJP test case, I'll let you know when I'm done, and I'll update this PR with my fixes.

@fl4via fl4via removed the failed CI Introduced new regession(s) during CI check label Feb 12, 2024
@fl4via fl4via added waiting PR update Awaiting PR update(s) from contributor before merging and removed next release This PR will be merged before next release or has already been merged (for payload double check) labels Feb 13, 2024
@fl4via
Copy link
Member

fl4via commented Feb 13, 2024

I have just updated the PR with my latest fixes. I can see that proxy http2 scenario is not working, and the reason for this is that the code goes through a different path when serving the request, and it ends up not invoking HttpRequestParser. It might be an issue in the tests or in the code, at this point I am not sure, and I will need to investigate further to find out.

@fl4via fl4via changed the title [UNDERTOW-2312] multibytes language in URL request to http/https are … WIP [UNDERTOW-2312] multibytes language in URL request to http/https are … Feb 13, 2024
@fl4via fl4via added the failed CI Introduced new regession(s) during CI check label Feb 13, 2024
@baranowb
Copy link
Contributor Author

Sometimes tests mimic desired behavior that is expected, because setup is not attainable (it seems)

@fl4via fl4via removed waiting PR update Awaiting PR update(s) from contributor before merging under verification Currently being verified (running tests, reviewing) before posting a review to contributor failed CI Introduced new regession(s) during CI check labels Apr 18, 2024
@fl4via fl4via force-pushed the UNDERTOW-2312 branch 3 times, most recently from 55e928e to d2ee06d Compare April 18, 2024 20:56
@fl4via fl4via added the next release This PR will be merged before next release or has already been merged (for payload double check) label Apr 18, 2024
@fl4via fl4via changed the title WIP [UNDERTOW-2312] multibytes language in URL request to http/https are … [UNDERTOW-2312] multibytes language in URL request to http/https are … Apr 18, 2024
@fl4via fl4via added the waiting CI check Ready to be merged but waiting for CI check label Apr 18, 2024
@fl4via fl4via removed next release This PR will be merged before next release or has already been merged (for payload double check) waiting CI check Ready to be merged but waiting for CI check labels Apr 18, 2024
@baranowb baranowb requested a review from fl4via July 16, 2024 09:47
@fl4via fl4via force-pushed the UNDERTOW-2312 branch 2 times, most recently from b4e9488 to 1eda14b Compare August 20, 2024 22:18
@fl4via fl4via added next release This PR will be merged before next release or has already been merged (for payload double check) waiting CI check Ready to be merged but waiting for CI check and removed waiting PR update Awaiting PR update(s) from contributor before merging failed CI Introduced new regession(s) during CI check waiting peer review PRs that edit core classes might require an extra review labels Aug 20, 2024
@fl4via
[UNDERTOW-2312] Add test for access log with unescaped characters. Al…
d66f660 
…so: make the test pass on the modes for HTTP2 upgrade and AJP

Signed-off-by: Flavia Rainone <[email protected]>
Signed-off-by: fl4via <[email protected]>
@fl4via fl4via added waiting PR update Awaiting PR update(s) from contributor before merging failed CI Introduced new regession(s) during CI check and removed waiting CI check Ready to be merged but waiting for CI check labels Aug 21, 2024
@fl4via
Copy link
Member

fl4via commented Aug 21, 2024

The problem in Windows CI is fixed. But now we have a problem in the new LotsOfQueryParamtersTestCase test. Investigating.

@fl4via fl4via added waiting CI check Ready to be merged but waiting for CI check and removed failed CI Introduced new regession(s) during CI check waiting PR update Awaiting PR update(s) from contributor before merging labels Aug 21, 2024
@fl4via
Copy link
Member

fl4via commented Aug 21, 2024

The cause of the failures is explained in the last commit. I decided to comment it there so it can be easily traced back in the future, since it is a corner case that could have been easily overlooked, weren't it for LostOfQueryParametersTestCase

@fl4via fl4via changed the title WIP [UNDERTOW-2312] multibytes language in URL request to http/https are … [UNDERTOW-2312] multibytes language in URL request to http/https are … Aug 21, 2024
jasondlee
jasondlee approved these changes Aug 21, 2024
View reviewed changes
Copy link
Contributor

@jasondlee jasondlee left a comment

Choose a reason for hiding this comment

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

These changes look good to me. Side note: I like the addition of final. :)

@fl4via
[UNDERTOW-2312] Fix ALLOW_UNESCAPED_CHARACTERS_IN_URL with query stri…
baaab68 
…ngs in Http2 upgrade

The  new code caused ALLOW_UNESCAPED_CHARACTERS_IN_URL to not work correctly in HTTP2 upgrade scenarios when the characters are in the query. The reason for that is that Http2UpgradeHandler passes
decode as false when asking Connectors to parse the request path, assuming that otherwise the decode would have been done twice. While that may the true for the parsing of the path, it is not the case when Connectors is parsing query params to set them one-by-one in the HttpServerExchange.This caused LotsOfQueryParametersTestCase to fail when run with HTTP2 Upgrade config.

Signed-off-by: Flavia Rainone <[email protected]>
@fl4via fl4via removed the waiting CI check Ready to be merged but waiting for CI check label Aug 22, 2024
@fl4via fl4via merged commit 1af12df into undertow-io:main Aug 22, 2024
34 checks passed
This was referenced Aug 22, 2024
Merged
Merged
@fl4via fl4via removed the next release This PR will be merged before next release or has already been merged (for payload double check) label Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fl4via fl4via fl4via approved these changes

@jasondlee jasondlee jasondlee approved these changes

@ropalka ropalka Awaiting requested review from ropalka

Assignees
No one assigned
Labels
bug fix Contains bug fix(es)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@baranowb @fl4via @jasondlee