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

fix(jetty12): more align RequestURI attr creation with that of Jetty 11 #7

Merged
merged 2 commits into from
Apr 28, 2024

Conversation

marinedayo
Copy link
Contributor

Ref: #4 (comment)

Why

By the following pull request, now provides the path part of the URI in Jetty 12 as well.

However, HttpURI::getDecodedPath method returns a decoded path, so the behavior is different from other environments, such as Jetty 11.

What

  • In Jetty 12 module, use HttpURI::getPath method instead, which returns the raw format
  • Add tests for RequestURI attribute creation results

Provide the path part of the URI in raw format.

Signed-off-by: You Aoki <165284116+marinedayo@users.noreply.github.com>
@marinedayo marinedayo force-pushed the more-align-requesturi-in-jetty-12 branch from b4a0fb0 to 8f01a11 Compare April 14, 2024 09:41
@@ -68,7 +68,7 @@ public void getRequest() throws Exception {

@Test
public void eventGoesToAppenders() throws Exception {
URL url = new URL(JETTY_FIXTURE.getUrl());
URL url = new URL("http://localhost:" + RANDOM_SERVER_PORT + "/path/foo%20bar;param?query#fragment");
Copy link
Contributor

@sondemar sondemar Apr 17, 2024

Choose a reason for hiding this comment

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

optional: How about leaving it as it was to maintain the same entry point?

Suggested change
URL url = new URL("http://localhost:" + RANDOM_SERVER_PORT + "/path/foo%20bar;param?query#fragment");
URL url = new URL(JETTY_FIXTURE.getUrl() + "path/foo%20bar;param?query#fragment");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your reply.

53b341a done.

@@ -68,7 +68,7 @@ public void getRequest() throws Exception {

@Test
public void eventGoesToAppenders() throws Exception {
URL url = new URL(JETTY_FIXTURE.getUrl());
URL url = new URL("http://localhost:" + RANDOM_SERVER_PORT + "/path/foo%20bar;param?query#fragment");
Copy link
Contributor

Choose a reason for hiding this comment

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

the same as above

Signed-off-by: You Aoki <165284116+marinedayo@users.noreply.github.com>
@marinedayo
Copy link
Contributor Author

Hi @ceki , could you please review and merge? Thanks.

@ceki ceki merged commit ccca6e8 into qos-ch:main Apr 28, 2024
1 check passed
@marinedayo marinedayo deleted the more-align-requesturi-in-jetty-12 branch April 29, 2024 12:50
@marinedayo
Copy link
Contributor Author

Thank you.

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.

3 participants