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-2245]javax.servlet.forward.mapping request attribute is not… #1446

Merged
merged 1 commit into from Apr 4, 2023
Merged

[UNDERTOW-2245]javax.servlet.forward.mapping request attribute is not… #1446

merged 1 commit into from Apr 4, 2023

Conversation

moulalis
Copy link
Contributor

@moulalis moulalis commented Mar 3, 2023 •
edited by fl4via

@moulalis moulalis marked this pull request as ready for review March 3, 2023 15:59
fl4via
fl4via requested changes Mar 22, 2023
View reviewed changes
Copy link
Member

@fl4via fl4via left a comment

Choose a reason for hiding this comment

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

Overall it looks very good, but there are some small fixes and cosmetic changes that need to be applied here.

servlet/src/test/java/io/undertow/servlet/test/util/DeploymentUtils.java Outdated
@@ -79,5 +81,10 @@ public static Deployment setupServlet(final ServletExtension servletExtension, f
}



public static boolean isContainString(String fullString, String partString){
Copy link
Member

Choose a reason for hiding this comment

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

I would remove this method from here, since deployment utils is an utility class for setting up test deployments. You can add an Utils method to the dispatcher package if you want to keep the utility method somewhere.
Also, I'm sorry if I sound too picky, but I would also rename the method to containsString or maybe even containsWord, since you used the \b in the pattern.

servlet/src/main/java/io/undertow/servlet/util/DispatchUtils.java Outdated
@@ -86,6 +89,7 @@ public static ServletPathMatch dispatchForward(final String path,
requestImpl.setAttribute(FORWARD_SERVLET_PATH, requestImpl.getServletPath());
requestImpl.setAttribute(FORWARD_PATH_INFO, requestImpl.getPathInfo());
requestImpl.setAttribute(FORWARD_QUERY_STRING, requestImpl.getQueryString());
requestImpl.setAttribute(FORWARD_MAPPING,requestImpl.getHttpServletMapping());
Copy link
Member

Choose a reason for hiding this comment

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

just to keep the style (I'm surprised that the code style enforcer didn't catch this), please add a space after commas, two dots, etc.

Copy link
Member

Choose a reason for hiding this comment

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

I see we are missing the new attribute at dispatchError (method of this same class as well), please add FORWARD_MAPPING there (the if statement at line 50)

servlet/src/test/java/io/undertow/servlet/test/dispatcher/DispatcherForwardTestCase.java Outdated
assertTrue(DeploymentUtils.isContainString(response,"jakarta.servlet.forward.context_path"));
assertTrue(DeploymentUtils.isContainString(response,"jakarta.servlet.forward.servlet_path"));
assertTrue(DeploymentUtils.isContainString(response,"jakarta.servlet.forward.request_uri"));
assertTrue(DeploymentUtils.isContainString(response,"jakarta.servlet.forward.mapping"));
Copy link
Member

Choose a reason for hiding this comment

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

what about path info and query string attributes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @fl4via As discussed this is causing test failure, Spec need to review for this to fix.

servlet/src/test/java/io/undertow/servlet/test/dispatcher/DispatcherIncludeTestCase.java Outdated
assertTrue(DeploymentUtils.isContainString(response,"jakarta.servlet.include.context_path"));
assertTrue(DeploymentUtils.isContainString(response,"jakarta.servlet.include.servlet_path"));
assertTrue(DeploymentUtils.isContainString(response,"jakarta.servlet.include.request_uri"));
assertTrue(DeploymentUtils.isContainString(response,"jakarta.servlet.include.mapping"));
Copy link
Member

Choose a reason for hiding this comment

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

missing path info and query string

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @fl4via As discussed this is causing test failure, Spec need to review for this to fix.

@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) waiting PR update Awaiting PR update(s) from contributor before merging labels Mar 22, 2023
@fl4via fl4via mentioned this pull request Mar 22, 2023
Merged
@moulalis
Copy link
Contributor Author

Hi @fl4via
I've updated the PR. Please review.
Thanks.

@moulalis moulalis force-pushed the UNDERTOW-2245_master branch 2 times, most recently from 88e64c6 to 8787bc7 Compare March 24, 2023 10:30
@soul2zimate
Copy link
Contributor

Hi @moulalis, the new added tests failed in CI.

Error:  Failures: 
Error:    DispatcherForwardTestCase.testDisptacherServletForward:398 COM01003: Internal error: Assertion failure: Expected boolean value to be false
Error:    DispatcherIncludeTestCase.testDisptacherServletInclude:291 COM01003: Internal error: Assertion failure: Expected boolean value to be false

@moulalis
Copy link
Contributor Author

Hi @moulalis, the new added tests failed in CI.

Error:  Failures: 
Error:    DispatcherForwardTestCase.testDisptacherServletForward:398 COM01003: Internal error: Assertion failure: Expected boolean value to be false
Error:    DispatcherIncludeTestCase.testDisptacherServletInclude:291 COM01003: Internal error: Assertion failure: Expected boolean value to be false

Looking into it.

@fl4via fl4via removed the next release This PR will be merged before next release or has already been merged (for payload double check) label Mar 25, 2023
@moulalis
Copy link
Contributor Author

@fl4via I've updated the PR. Please review it.
Thanks.

fl4via
fl4via requested changes Apr 4, 2023
View reviewed changes
servlet/src/test/java/io/undertow/servlet/test/dispatcher/util/DispatcherUtil.java Outdated
@@ -0,0 +1,40 @@
/*
* JBoss, Home of Professional Open Source.
* Copyright 2014 Red Hat, Inc., and individual contributors
Copy link
Member

Choose a reason for hiding this comment

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

Please, update the year to 20223.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fl4via I've updated the changes to 2023.

servlet/src/test/java/io/undertow/servlet/test/dispatcher/NextServlet.java Outdated
@@ -0,0 +1,23 @@
package io.undertow.servlet.test.dispatcher;
Copy link
Member

Choose a reason for hiding this comment

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

Missing license header.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fl4via I've updated the changes.
Thanks.

@moulalis moulalis force-pushed the UNDERTOW-2245_master branch 3 times, most recently from 410ec92 to f5c34ff Compare April 4, 2023 06:42
soul2zimate
soul2zimate reviewed Apr 4, 2023
View reviewed changes
servlet/src/test/java/io/undertow/servlet/test/dispatcher/DispatcherIncludeServlet.java
@@ -0,0 +1,16 @@
package io.undertow.servlet.test.dispatcher;
Copy link
Contributor

Choose a reason for hiding this comment

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

This also needs a license header.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Thanks

Copy link
Contributor

Choose a reason for hiding this comment

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

s/2014/2023

@moulalis
[UNDERTOW-2245]javax.servlet.forward.mapping request attribute is not…
4810416 
… available in servlets forwarded with RequestDispatcher
@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 waiting CI check Ready to be merged but waiting for CI check labels Apr 4, 2023
@fl4via fl4via merged commit 56d4ef3 into undertow-io:master Apr 4, 2023
25 checks passed
@fl4via fl4via removed the next release This PR will be merged before next release or has already been merged (for payload double check) label May 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@soul2zimate soul2zimate soul2zimate left review comments

@fl4via fl4via fl4via approved these changes

Assignees
No one assigned
Labels
bug fix Contains bug fix(es)
Projects
None yet
Milestone
No milestone
4 participants
@moulalis @soul2zimate @fl4via @davidklika