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

Provide better lifecyle for WebMvcConfigurer.configurePathMatch #26427

Closed
philwebb opened this issue Jan 21, 2021 · 5 comments
Closed

Provide better lifecyle for WebMvcConfigurer.configurePathMatch #26427

philwebb opened this issue Jan 21, 2021 · 5 comments
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Milestone

Comments

@philwebb
Copy link
Member

Currently WebMvcConfigurer.configurePathMatch(...) is called as a side-effect of WebMvcConfigurationSupport.getPathMatchConfigurer(). This makes it quite hard to use in @Bean method since it's hard to tell if the @Bean method or the configurePathMatch(...) method will be called first.

You can see an example of this in Spring Data Rest where the restHandlerMapping bean would ideally have a PatternParser on the delegates before the afterPropertiesSet() method is called. The current solution of overriding configurePathMatch and calling setPatternParser directly is brittle because the parser is needed before afterPropertiesSet() is called. This leads to issues such as #26415

It's possible to work-around the issue by adding @DependsOn("mvcPathMatcher") which will indirectly trigger the WebMvcConfigurationSupport.getPathMatchConfigurer() method, but this isn't ideal.

I wonder if we could make PathMatchConfigurer or the PatternParser a @Bean so that it can be injected directly if it's needed?

@rstoyanchev
Copy link
Contributor

We could expose an mvcPatternParser bean that mirrors the mvcPathMatcher() bean and exposes a global PathPatternParser for use throughout the application. The restHandlerMapping bean in Spring Data REST would then make use of that and configure its own handler mappings before calling afterPropertiesSet.

The one catch is that by doing this Spring Data REST opts into always using parsed patterns, which may be a reasonable choice I think. It performs better and supports largely the same syntax (except for notably ** in any but the last segment of the path) so it should be transparent. What do you think @odrotbohm?

@rstoyanchev rstoyanchev self-assigned this Jan 22, 2021
@rstoyanchev rstoyanchev added in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Jan 22, 2021
@rstoyanchev rstoyanchev added this to the 5.3.4 milestone Jan 22, 2021
@rstoyanchev rstoyanchev added the status: waiting-for-triage An issue we've not yet triaged or decided on label Jan 22, 2021
rstoyanchev added a commit that referenced this issue Jan 22, 2021
This commit ensures the PathPatternParser cannot be set after request mappings
have been initialized when it is too late.

See gh-26427
@rstoyanchev
Copy link
Contributor

I've added assertions in HandlerMapping implementations to ensure the PathPatternParser is rejected if it is too late and I've also added an mvcPatternParser bean. Spring Data REST can inject this bean into its mappings, effectively always opting into parsed patterns.

@rstoyanchev rstoyanchev removed the status: waiting-for-triage An issue we've not yet triaged or decided on label Jan 22, 2021
odrotbohm added a commit to spring-projects/spring-data-rest that referenced this issue Jan 26, 2021
…ing.

Instead of registering the PathPatternParser on DelegatingHandlerMapping via WebMvcConfigurer.configurePathMatch(…) we now consume the bean exposed in context of the fix for spring-projects/spring-framework#26427.
odrotbohm added a commit to spring-projects/spring-data-rest that referenced this issue Jan 26, 2021
…ing.

Instead of registering the PathPatternParser on DelegatingHandlerMapping via WebMvcConfigurer.configurePathMatch(…) we now consume the bean exposed in context of the fix for spring-projects/spring-framework#26427.

We also use the newly introduced RequestMappingInfo.mutate() to add our customizations of the produces clause for Spring Data REST's mappings.

Fixes GH-1965.
odrotbohm added a commit to spring-projects/spring-data-rest that referenced this issue Jan 26, 2021
…ing.

Instead of registering the PathPatternParser on DelegatingHandlerMapping via WebMvcConfigurer.configurePathMatch(…) we now consume the bean exposed in context of the fix for spring-projects/spring-framework#26427.

We also use the newly introduced RequestMappingInfo.mutate() to add our customizations of the produces clause for Spring Data REST's mappings.

Fixes GH-1965.
@rstoyanchev
Copy link
Contributor

@odrotbohm are the current changes sufficient in which case we can close this?

@odrotbohm
Copy link
Member

I think we're set. Fixes already in Spring Data REST master and supported branches for quite a while.

@rstoyanchev
Copy link
Contributor

Okay thanks.

This was referenced Mar 12, 2021
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: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

4 participants