Skip to content

PathPattern doesn't extract pathWithinHandlerMapping correctly for non-pattern URLs #25174

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

Closed
rstoyanchev opened this issue Jun 2, 2020 · 3 comments
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) status: invalid An issue that we don't feel is valid

Comments

@rstoyanchev
Copy link
Contributor

This is defined as:

Name of the {@link HttpServletRequest} attribute that contains the path within the handler mapping, in case of a pattern match, or the full relevant URI (typically within the DispatcherServlet's mapping) else.

However, PathPattern defines that case as an empty string:

* <li>'{@code /docs/cvs/commit.html}' and '{@code /docs/cvs/commit.html} &rarr; ''</li>

I don't know if there was a reason for the discrepancy or if it accidental. @bclozel and @aclement do you recall anything more?

I think it would be okay to correct the behavior. The only place where this is used in WebFlux is for static resources where are typically mapped with a pattern. Now that we are adding support for PathPattenr in Spring MVC with #24945 we should correct it to make it consistent and easier to switch from AntPathMatcher to PathPatternParser in Spring MVC.

@rstoyanchev rstoyanchev self-assigned this Jun 2, 2020
@rstoyanchev rstoyanchev added this to the 5.3 M1 milestone Jun 2, 2020
@rstoyanchev rstoyanchev added in: web Issues in web modules (web, webmvc, webflux, websocket) type: bug A general bug labels Jun 2, 2020
@bclozel
Copy link
Member

bclozel commented Jun 2, 2020

I can see a few tests/corner cases that we need to check for consistency once that change is in.

I don't remember a particular reason behind this behavior, besides that if there is no pattern defined, there can't be any path within a non-existing pattern. Also with that change, would those two examples yield the same result?

  • pattern "/**", path "/static/image.png" -> "/static/image.png"
  • pattern "/static/image.png", path "/static/image.png" -> "/static/image.png"

@aclement
Copy link
Contributor

aclement commented Jun 4, 2020

@rstoyanchev Sorry Rossen, I can't remember. I feel like we spent a lot of timing crafting things here though so I'm maybe a little surprised if it was accidental.

@rstoyanchev rstoyanchev modified the milestones: 5.3 M1, 5.3 M2 Jun 15, 2020
@rstoyanchev rstoyanchev modified the milestones: 5.3 M2, 5.3 RC1 Aug 4, 2020
@rstoyanchev
Copy link
Contributor Author

Taking another look, PathPattern and AntPathMatcher are consistent in this regard. In SimpleUrlHandlerMapping the case when the match is not a pattern is handled as a direct hit and the path is the pathWithinHandlerMapping so no issue there. For RequestMappingHandlerMapping this is less important and is always the full lookupPath. So everything seems okay after all.

@rstoyanchev rstoyanchev removed this from the 5.3 RC1 milestone Sep 13, 2020
@rstoyanchev rstoyanchev added status: invalid An issue that we don't feel is valid and removed type: bug A general bug labels Sep 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) status: invalid An issue that we don't feel is valid
Projects
None yet
Development

No branches or pull requests

3 participants