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

Wrong value of RequestDispatcher.FORWARD_CONTEXT_PATH attribute on root context #9123

Merged

Conversation

joakime
Copy link
Contributor

@joakime joakime commented Jan 3, 2023

Signed-off-by: Joakim Erdfelt joakim.erdfelt@gmail.com

* Fixes #9119 - uses proper context path that
  satisfies the root context rules of the servlet
  spec

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime joakime added Bug For general bugs on Jetty side Specification For all industry Specifications (IETF / Servlet / etc) labels Jan 3, 2023
@joakime joakime added this to the 10.0.x milestone Jan 3, 2023
@joakime joakime requested review from gregw and sbordet January 3, 2023 21:51
@joakime joakime self-assigned this Jan 3, 2023
@@ -878,35 +1003,56 @@ protected void doGet(HttpServletRequest request, HttpServletResponse response) t
}
}

public static class AssertForwardServlet extends HttpServlet implements Servlet
public static class DumpForwardServlet extends HttpServlet
Copy link
Contributor Author

@joakime joakime Jan 3, 2023

Choose a reason for hiding this comment

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

I made this generic, so it can be used from different test case setups.
It's a better idea to assert values in the same thread the test is running in anyway.

grgrzybek
grgrzybek previously approved these changes Jan 4, 2023
sbordet
sbordet previously approved these changes Jan 9, 2023
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

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

I'm trying to work out why the code was written that way in the first place? I can't see why as the request getContextPath also returns the encoded value???

Might be worthwhile to add a test of doing a forward from a cross context include.

@olamy can we run the TCK against this branch before merging?

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime joakime dismissed stale reviews from sbordet and grgrzybek via 9ced89d January 9, 2023 22:11
@gregw
Copy link
Contributor

gregw commented Jan 9, 2023

I've approved changes, but let's keep issue open until TCK tests are double checked.

@joakime
Copy link
Contributor Author

joakime commented Jan 16, 2023

@olamy can you test this branch with the TCK?

@olamy
Copy link
Member

olamy commented Jan 20, 2023

all good regarding TCK build here https://jenkins.webtide.net/job/tck/job/servlettck-run-jetty-10.0.x/2044/
please note 2 failing tests are due to outdated certificate included in the TCK distribution.

@joakime joakime merged commit 4993291 into jetty-10.0.x Jan 20, 2023
@joakime joakime deleted the fix/jetty-10.0.x/9119-forward-context-path-attribute branch January 20, 2023 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For general bugs on Jetty side Specification For all industry Specifications (IETF / Servlet / etc)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong value of javax.servlet.forward.context_path attribute
5 participants