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

Update comment in Javadoc of ServletRequestPathFilter DispatcherServlet relating to DispatcherServlet #30014

Closed
wants to merge 1 commit into from

Conversation

sergiuprdn
Copy link
Contributor

The following behavior documented by org.springframework.web.filter.ServletRequestPathFilter does not hold true with current version:

DispatcherServlet will parse and cache the RequestPath if it detects that parsed PathPatterns are enabled for any HandlerMapping but it will skip doing that if ServletRequestPathUtils#PATH_ATTRIBUTE already exists.

This PR adds the logic to skip parse and cache RequestPath if ServletRequestPathUtils.PATH_ATTRIBUTE already exists.

The exception where multiple parsing must happen is for Forward requests.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Feb 22, 2023
@rstoyanchev rstoyanchev added the in: web Issues in web modules (web, webmvc, webflux, websocket) label Feb 24, 2023
Copy link
Contributor

@rstoyanchev rstoyanchev left a comment

Choose a reason for hiding this comment

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

Thanks for bringing this up. You're right that the Javadoc reflects original intent rather than current behavior.

For a FORWARD, the filter would also get involved, so technically the DispatcherServlet can always decide to stick with the existing one. The problem is that when there is no Filter, on FORWARD we would find a previous value, and the path would need to be parsed again. This is why in the end DispatcherServlet ended up with always doing its own parsing, while the Filter is there mostly if anything in the Filter chain relies on the parsed path.

There are also other scenarios with ERROR and ASYNC dispatches, and I'm not sure it's worth doing anything more here. We might just have to correct the Javadoc of the filter accordingly.

@rstoyanchev rstoyanchev self-assigned this Feb 24, 2023
@rstoyanchev rstoyanchev added this to the 6.0.6 milestone Feb 24, 2023
@rstoyanchev rstoyanchev added type: documentation A documentation task and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Feb 24, 2023
@rstoyanchev rstoyanchev changed the title DispatcherServlet skip parse and cache RequestPath if ServletRequestPathUtils.PATH_ATTRIBUTE already exists Update comment Javadoc of ServletRequestPathFilter DispatcherServlet relating to DispatcherServlet Feb 24, 2023
@rstoyanchev rstoyanchev changed the title Update comment Javadoc of ServletRequestPathFilter DispatcherServlet relating to DispatcherServlet Update comment in Javadoc of ServletRequestPathFilter DispatcherServlet relating to DispatcherServlet Feb 24, 2023
@sergiuprdn
Copy link
Contributor Author

Updated javadoc as indicated by PR title. Please check

rstoyanchev pushed a commit that referenced this pull request Feb 27, 2023
mdeinum pushed a commit to mdeinum/spring-framework that referenced this pull request Jun 29, 2023
mdeinum pushed a commit to mdeinum/spring-framework that referenced this pull request Jun 29, 2023
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) type: documentation A documentation task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants