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 #11815 HttpServletMapping #11829

Merged
merged 3 commits into from
May 25, 2024

Conversation

gregw
Copy link
Contributor

@gregw gregw commented May 22, 2024

Implemented a test that uses the examples from the javadoc Fixed the matchValue for PREFIX matches

Implemented a test that uses the examples from the javadoc
Fixed the matchValue for PREFIX matches
@gregw gregw requested a review from janbartel May 22, 2024 01:55
@gregw gregw added the Specification For all industry Specifications (IETF / Servlet / etc) label May 22, 2024
@gregw
Copy link
Contributor Author

gregw commented May 22, 2024

I'm inclined not to port back to EE10 as nobody has complained and we passed the TCK.

@gregw
Copy link
Contributor Author

gregw commented May 23, 2024

@janbartel @lachlan-roberts the CI failures are just the asciidoc stuff... so can I get a review please.

@gregw
Copy link
Contributor Author

gregw commented May 23, 2024

@janbartel @lachlan-roberts nudge!

_pathInfo = matchedPath != null
? matchedPath.getPathInfo()
: _servletPath.length() == pathInContext.length() ? null : pathInContext.substring(_servletPath.length());
_matchValue = _pathInfo == null ? "" : _pathInfo.startsWith("/") ? _pathInfo.substring(1) : _pathInfo;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this using _pathInfo and not pathInContext?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The spec requires that for in a context "/ctx", with a pattern "/foo/*" then for a request to "/ctx/foo/info", the contextPath will be "/ctx", the servletPath will be "/foo" and the pathInfo "/info". We had previously implemented that the match value was based on the servletPath (so "foo"), but it needs to be based on the part that matched the *, i.e. the pathInfo without the leading slash (so "info").

The path in context is "/foo/info", so that is servletPath + pathInfo and is not what we want for the matchValue.

@gregw gregw merged commit a84f99b into jetty-12.1.x May 25, 2024
10 checks passed
@gregw gregw deleted the fix/jetty-12.1.x/11815/HttpServletMapping branch May 25, 2024 00:35
@olamy olamy mentioned this pull request Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Specification For all industry Specifications (IETF / Servlet / etc)
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

3 participants