Make i18n and URL-mappings framework beans overridable (@ConditionalOnMissingBean)#15916
Conversation
@EnableWebMvc is a Spring Framework annotation that is not meant to be combined with Spring Boot. Importing it (via DelegatingWebMvcConfiguration) disables Boot's WebMvcAutoConfiguration MVC setup (which is @ConditionalOnMissingBean(WebMvcConfigurationSupport)) and registers MVC infrastructure beans in the user-configuration phase, ahead of all auto-configuration. A concrete symptom is the localeResolver bean: WebMvcConfigurationSupport registers an AcceptHeaderLocaleResolver in the user phase, so grails-i18n's @AutoConfigureBefore(WebMvcAutoConfiguration) SessionLocaleResolver can only ever override it (triggering a bean-definition-override) rather than supersede it cleanly. Remove the auto-injected @EnableWebMvc so Boot's WebMvcAutoConfiguration drives MVC setup.
…ctive
With @EnableWebMvc no longer auto-injected, Boot's WebMvcAutoConfiguration becomes
active and registers an OrderedRequestContextFilter (order -105). That runs between
GrailsWebRequestFilter (-150) and the Spring Security chain (-100) and rebinds a plain
ServletRequestAttributes, so security-chain code that expects a GrailsWebRequest fails
with a ClassCastException (HTTP 500 on every request).
Boot's filter is @ConditionalOnMissingBean({RequestContextListener, RequestContextFilter}),
so make GrailsWebRequestFilter extend RequestContextFilter (it already overrides
shouldNotFilterAsyncDispatch/ErrorDispatch identically and binds a richer request context)
and expose it as a bean that the FilterRegistrationBean wraps. Boot then backs off in
favour of Grails' request-context binding.
With @EnableWebMvc no longer auto-injected, Spring Boot's WebMvcAutoConfiguration is active and contributes a defaultViewResolver (InternalResourceViewResolver). For a Grails app that does not use JSP/InternalResource views (e.g. a REST/JSON-views app) this catch-all resolves any unmatched view name to a servlet forward, yielding "Circular view path [index]: would dispatch back to the current handler URL" (HTTP 500). grails-gsp already strips this bean for GSP apps via its own registrar, but a JSON-only app never loads grails-gsp. Add a core auto-configuration (ordered after WebMvcAutoConfiguration) that removes defaultViewResolver for every Grails servlet web application, so Grails' own view resolution is used. Disable with grails.web.removeDefaultViewResolverBean=false. Fixes the graphql, issue-views-182 and views-functional-tests example apps.
GrailsViewResolverAutoConfiguration (grails-controllers) already removes Boot's defaultViewResolver for every Grails servlet web app, so grails-gsp's registrar no longer needs to do it too. Drop the duplicate defaultViewResolver removal (and its spring.gsp.removeDefaultViewResolverBean flag) from grails-gsp, leaving only the GSP-specific behaviour: replacing the viewResolver bean with an alias to gspViewResolver. Rename the registrar to ReplaceViewResolverRegistrar to reflect its remaining role. No behaviour change: GSP apps still have defaultViewResolver removed (now via the core auto-configuration) and gspViewResolver aliased as viewResolver.
…Filter for Grails apps
Resolve upgrade-notes conflict in upgrading80x.adoc: keep apache/8.0.x's 'MIME Types Are Now Framework Defaults' and 'Accept Header Honored for All Clients' sections and renumber the appended '@EnableWebMvc No Longer Added' section to 30 (subsections 30.1/30.2), giving sequential 26-30 numbering.
…eWebMvc # Conflicts: # grails-doc/src/en/guide/upgrading/upgrading80x.adoc
…beans - Honour the deprecated spring.gsp.removeDefaultViewResolverBean property as a fallback for grails.web.removeDefaultViewResolverBean, logging a deprecation warning when the old name is set - Guard grailsWebRequestFilter and webMvcConfig with @ConditionalOnMissingBean so user-defined beans take precedence - Update the 8.0 upgrade notes and add test coverage for the property matrix and the bean back-off behavior
Guard framework beans with @ConditionalOnMissingBean so an application- or plugin-defined bean takes precedence cleanly instead of relying on bean-definition overriding: - i18n: localeResolver, localeChangeInterceptor, messageSource - URL mappings: grailsCorsFilter, urlMappingsErrorPageCustomizer, urlMappingsInfoHandlerAdapter The beans still register by default. The localeResolver guard requires the @EnableWebMvc removal (apache#13863): with @EnableWebMvc present, WebMvcConfigurationSupport contributes its own localeResolver bean that would win the @ConditionalOnMissingBean race and silently replace Grails' SessionLocaleResolver. Adds auto-configuration test coverage for both modules (default registration, property toggles, and user-bean back-off) and documents the change in the 8.0 upgrade notes.
jdaugherty
left a comment
jdaugherty
left a comment
There was a problem hiding this comment.
I don't see anything immediately wrong with this. I merged the latest 8.0.x branch into this one to see if there are any notable test failures. I also asked AI to review it and it some minor comments that we should probably discuss. Overall, approving. We should wait to merge it until we have a second review though.
| } | ||
|
|
||
| @Bean(AbstractApplicationContext.MESSAGE_SOURCE_BEAN_NAME) | ||
| @ConditionalOnMissingBean(name = AbstractApplicationContext.MESSAGE_SOURCE_BEAN_NAME) |
There was a problem hiding this comment.
Consider @ConditionalOnMissingBean(name = AbstractApplicationContext.MESSAGE_SOURCE_BEAN_NAME, search = SearchStrategy.CURRENT) here. Boot's own MessageSourceAutoConfiguration scopes its equivalent guard to SearchStrategy.CURRENT specifically because a messageSource is commonly present in a parent context, and each child context should still get its own. With the default search strategy (ancestors included), any hierarchical deployment would cause the Grails PluginAwareResourceBundleMessageSource to silently back off in favor of the parent's plain message source, losing plugin message-bundle resolution. The same consideration applies (more weakly) to localeResolver and localeChangeInterceptor above, but messageSource is the one with a real hierarchy precedent.
There was a problem hiding this comment.
Applied — and to all three beans, not just messageSource. DispatcherServlet resolves localeResolver/localeChangeInterceptor from its own context, so the parent-suppression problem applies with the same force there: a child context whose bean backed off would fall back to DispatcherServlet's default AcceptHeaderLocaleResolver, not the parent's bean. Also verified Boot 4 no longer contributes a WebMvc localeResolver at all (it did in Boot 3), so SearchStrategy.CURRENT has no auto-config interplay to worry about. Added a parent-context test covering all three.
|
|
||
| ==== 32. More Framework Beans Are Cleanly Overridable | ||
|
|
||
| Several Grails framework beans that were previously registered unconditionally are now guarded with `@ConditionalOnMissingBean`, so an application- or plugin-defined bean of the same name (or type) takes precedence cleanly instead of relying on bean-definition overriding: |
There was a problem hiding this comment.
The "or plugin-defined" claim is stronger than what @ConditionalOnMissingBean can deliver. Beans contributed by a plugin's doWithSpring (or an app's resources.groovy) are registered by GrailsApplicationPostProcessor, a plain (unordered) BeanDefinitionRegistryPostProcessor that runs after ConfigurationClassPostProcessor has already evaluated auto-configuration conditions. Those beans therefore still land via bean-definition overriding, exactly as before — the conditional only sees beans defined in @Configuration classes and other auto-configurations. Suggest qualifying this so users don't expect doWithSpring beans to trigger the back-off (e.g. "a bean defined in application configuration classes"), or explicitly noting that doWithSpring/resources.groovy beans continue to rely on overriding.
There was a problem hiding this comment.
Good catch — qualified. The section now scopes the clean back-off to @Bean methods in application configuration classes, and a NOTE spells out that doWithSpring/resources.groovy beans continue to take effect through bean-definition overriding, exactly as before. (Closing that gap is what #15934 does — once the retimed doWithSpring drain registers plugin beans before the conditions are evaluated, this note can be revisited.)
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 8.0.x #15916 +/- ##
==================================================
+ Coverage 49.3373% 49.3593% +0.0220%
- Complexity 16775 16777 +2
==================================================
Files 1985 1986 +1
Lines 93327 93336 +9
Branches 16336 16337 +1
==================================================
+ Hits 46045 46070 +25
+ Misses 40158 40139 -19
- Partials 7124 7127 +3
🚀 New features to boost your workflow:
|
Review feedback on the stacked PR apache#15916: the property is user-facing configuration, so the constant belongs in grails.config.Settings with the other grails.web.* keys rather than a package-private field.
- Scope the i18n @ConditionalOnMissingBean guards to SearchStrategy.CURRENT: DispatcherServlet resolves localeResolver/localeChangeInterceptor from its own context and Boot's MessageSourceAutoConfiguration scopes its messageSource guard the same way, so parent-context beans must not suppress the child's - Broaden the CORS guard to CorsFilter so any user CORS filter registration wins, not only a custom GrailsCorsFilter (two CORS filters never coexist) - Reference the new Settings.WEB_REMOVE_DEFAULT_VIEW_RESOLVER_BEAN constant (declared on the base branch) instead of a string literal - Qualify the upgrade-notes claim: the back-off applies to beans registered before auto-configuration (application @bean methods); doWithSpring and resources.groovy beans continue to rely on bean-definition overriding - Add trailing newlines to the touched build files - Tests: parent-context hierarchy coverage for the i18n beans and plain CorsFilter back-off coverage
…hub.com/codeconsole/grails-core into feat/overridable-i18n-urlmappings-beans
✅ All tests passed ✅🏷️ Commit: 1ea2031 Learn more about TestLens at testlens.app. |
3 participants
Summary
Guards several Grails framework beans with
@ConditionalOnMissingBeanso an application- or plugin-defined bean takes precedence cleanly, instead of relying on bean-definition overriding (which produces a startup warning, or fails outright whenspring.main.allow-bean-definition-overriding=false):I18nAutoConfiguration):localeResolver,localeChangeInterceptor,messageSourceUrlMappingsAutoConfiguration):grailsCorsFilter,urlMappingsErrorPageCustomizer,urlMappingsInfoHandlerAdapterAll beans still register by default — this only changes what happens when the user defines their own.
Depends on #13863
This PR is stacked on #13863 (removal of the auto-injected
@EnableWebMvc) and includes its commits; merge that first, after which this diff reduces to the changes described above.The dependency is real, not just mechanical: with
@EnableWebMvcpresent, Spring'sWebMvcConfigurationSupportcontributes its ownlocaleResolverbean, which would win the@ConditionalOnMissingBeanrace and silently replace Grails'SessionLocaleResolver. With #13863 in,I18nAutoConfigurationis ordered beforeWebMvcAutoConfiguration, so the Grails bean registers first and Boot's conditional bean defers to it. This is what #15751 attempted and could not achieve standalone.Context
Extracted from #15755 as an independently useful piece while the plugin-lifecycle discussion there is resolved (see the review discussion about unifying bean registration ahead of auto-configuration).
Testing
New auto-configuration specs for both modules covering default registration, the
grails.cors.filtertoggle, and user-bean back-off for every guarded bean. Both modules' full test suites and checkstyle/CodeNarc pass.