Apply SiteMesh 3 decoration at the bean-definition level#15964
Conversation
Spring Boot's ContentNegotiatingViewResolver — active for Grails servlet apps since @EnableWebMvc stopped being auto-added (apache#13863) — collects every ViewResolver bean while it initializes, force-creating the lazy jspViewResolver. When that happens before the SiteMesh BeanPostProcessor wrap is in effect, the raw resolver is captured and, sitting behind the highest-precedence viewResolver, renders every HTML request undecorated: layouts silently stop applying. Whether the race is hit depends on bean-creation order, so it varies with the plugin set from app to app. Sitemesh3ViewResolverDefinitionPostProcessor now rewrites the jspViewResolver bean definition itself into a GrailsSiteMeshViewResolver with the original definition embedded as the inner bean — mirroring the SiteMesh 2 module's GrailsLayoutViewResolverPostProcessor — so the decorating resolver is what gets instantiated no matter how early a consumer forces the bean into existence. The bean post-processor is kept as a safety net for instance-registered resolvers and to keep upstream's wrap-all post-processor from registering; upstream never re-wraps a SiteMeshViewResolver, so the mechanisms cannot double-decorate. The plugin's doWithSpring() is removed outright: the SiteMesh property defaults now come from a Sitemesh3EnvironmentPostProcessor registered in spring.factories (making the post-hoc PropertySourcesConfig reassignment unnecessary), and grailsRenderViewMutator is registered through beanRegistrar(), the modern replacement for the bean-builder DSL.
With doWithSpring() gone there is no bean-builder closure left — the one construct whose dynamic dispatch prevents @CompileStatic on descriptor classes — so the whole class can be statically compiled, rather than annotating individual methods the way GroovyPagesGrailsPlugin must.
The description implied the plugin replaces SiteMesh 2, but the two integrations coexist — when grails-layout is present its decoration wins and SiteMesh 3 stands down. Also drop the registration-site comment that duplicated Sitemesh3RenderViewMutator's own javadoc.
codeconsole
requested review from
borinquenkid,
jamesfredley,
jdaugherty,
matrei and
sbglasius
The declared BeanRegistrar return type already coerces the returned closure; the 'as BeanRegistrar' was redundant.
…n place The upstream post-processor warns at startup when it wrapped nothing, but with decoration applied at the bean-definition level a zero-wrap startup is the expected healthy state. Answer the check from the target bean's definition type so the lazy resolver is not forced into existence.
Sitemesh3ViewResolverDefinitionPostProcessor now extends upstream's SiteMeshViewResolverPostProcessor - the class behind sitemesh.viewResolver.wrapMode=bean-definition - and is registered under upstream's bean name, so upstream's own @ConditionalOnMissingBean guard defers to it. Together with the bean post-processor (which preempts the wrap-all and bean-instance registrations by type), every upstream wrap mode is covered by a Grails-flavoured implementation and the zero-wrap startup warning cannot arise from a dangling mode. The Grails implementation deliberately diverges from upstream on one point: upstream re-registers the unwrapped resolver as a separate named bean that stays visible to getBeansOfType sweeps - the same exposure this PR closes - so the original definition is embedded as an anonymous inner-bean definition instead. The wrapper definition now also honours the inherited dispatchMode and includeErrorPages settings.
|
Design refinement after field testing: the definition post-processor now extends upstream's One deliberate divergence from upstream's |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 8.0.x #15964 +/- ##
==================================================
+ Coverage 49.6092% 49.6126% +0.0034%
- Complexity 16955 16977 +22
==================================================
Files 1999 2001 +2
Lines 93791 93835 +44
Branches 16421 16434 +13
==================================================
+ Hits 46529 46554 +25
- Misses 40090 40110 +20
+ Partials 7172 7171 -1
🚀 New features to boost your workflow:
|
grails-gsp-spring-boot applied the SiteMesh 3 property defaults through Sitemesh3GrailsPlugin.getDefaultPropertySource, which moved to Sitemesh3EnvironmentPostProcessor with an environment-only signature. The method is now public for this consumer, which covers plain Spring Boot GSP contexts built without SpringApplication where no EnvironmentPostProcessor runs. The explicit null default layout the old call passed is replaced by reading the configured layout keys from the environment, matching the Grails path.
jdaugherty
left a comment
jdaugherty
left a comment
There was a problem hiding this comment.
Have you tested this with sitemesh2? We still technically support it via grails-layout and the bean wiring now occurs at a different level.
| * itself, and the source is appended with lowest precedence, so application | ||
| * configuration always wins.</p> | ||
| */ | ||
| public class Sitemesh3EnvironmentPostProcessor implements EnvironmentPostProcessor, Ordered { |
There was a problem hiding this comment.
Deliberate: this module keeps its Spring infrastructure in Java (CaptureAwareContentProcessor, Sitemesh3LayoutFinder, GrailsSiteMeshViewResolver are all Java), these classes have no Groovy idiom to benefit from, and Java puts them under checkstyle. As a bonus it avoids the grails.util.Environment / Spring Environment name clash that forces fully-qualified references in Groovy.
There was a problem hiding this comment.
The groovy idiom is the code is easier to read because of property references etc
grails-sitemesh3 is a drop-in replacement for grails-layout - the two modules are never installed together - yet the integration carried SiteMesh 2 accommodation logic: the ApplicationListener exclusions in both post-processors existed to detect and stand down from the SiteMesh 2 layout resolver, and the plugin javadoc promised coexistence semantics that bean-registrar ordering had in fact already broken. All of that is removed. Sitemesh3EnvironmentPostProcessor now fails startup with a clear message when grails-layout is detected on the classpath, making the mutual-exclusion contract explicit instead of cooperative. The already-decorating check now resolves classes with the bean factory's bean class loader (falling back to the default loader) rather than the loader of the grails-sitemesh3 jar, so an application-defined SiteMeshViewResolver subclass living in a child or restart class loader (e.g. devtools) is recognised instead of being double-wrapped. The legacy grails.views.layout.default fallback is retained - honouring existing configuration is what makes the replacement drop-in.
The fail-fast on grails-layout co-presence broke real configurations: grails-sitemesh3 arrives transitively through grails-dependencies-starter-web, so an application that declares grails-layout - the standard state for SiteMesh 2 applications upgrading to Grails 8, and half a dozen apps in grails-test-examples - ends up with both on the classpath and failed startup. The modules remain mutually exclusive by contract, but co-presence now degrades the way those applications already relied on: the SiteMesh 2 integration keeps decorating and this module's machinery stands down - one classpath check gating the definition post-processor, the bean post-processor and the render-view mutator registration (whose registrar bean would otherwise displace SiteMesh 2's via name-conflict precedence). Sitemesh3EnvironmentPostProcessor warns loudly with the exclusion instructions, and the tolerance is documented as subject to removal. Verified against grails-test-examples-spring-dependency-management (the combined-classpath app that failed CI) and grails-test-examples-mail.
|
The functional-test failures were caused by the mutual-exclusion fail-fast from ba2c2ec: it turns out a combined classpath is not a freak misconfiguration but the standard state for SiteMesh 2 applications — grails-sitemesh3 arrives transitively via 32bb997 keeps the mutual-exclusion contract but tolerates co-presence the way those applications already relied on: SiteMesh 2 keeps decorating and this module stands down — one Escalating to a hard fail-fast would need the in-tree apps fixed (exclude grails-sitemesh3 from the starter) and is really a separate policy decision — happy to prepare that as a follow-up if that's the direction you want. Verified locally: |
SiteMesh 3.3.0-M2 absorbed the mechanisms this branch had to carry itself, so the Grails layer shrinks to its genuinely Grails-specific parts: - Sitemesh3ViewResolverDefinitionPostProcessor delegates the rewrite to the upstream implementation, which now embeds the original definition as an anonymous inner bean and skips already-decorating targets with the bean class loader. The subclass keeps only the Grails guards (SiteMesh 2 stand-down, test-context beans, silent missing target), a factory-method return-type check the upstream guard cannot see, and preservation of the target's lazy-init flag. - The zero-wrap warning override reduces to the SiteMesh 2 branch; upstream now recognises definition-level decoration itself. The warning specs move with the behaviour (upstream carries its own), which also retires the JUL capture that 3.3.0-M2's switch to Commons Logging would have broken. - The upgrade notes drop the NoopSitemeshFilter placeholder, which 3.3.0-M2 removed - the integrations are mutually exclusive by configuration and the 'Filter sitemesh was not registered (disabled)' startup line is gone. Verified against 3.3.0-M2: module tests, checkstyle, codenarc, whole-repo compile, and rerun integration tests for app1, spring-dependency-management (combined SiteMesh 2/3 classpath) and mongodb-base.
This comment has been minimized.
This comment has been minimized.
2 participants
Summary
Since #13863 stopped auto-adding
@EnableWebMvc, Spring Boot'sWebMvcAutoConfigurationis active for Grails servlet apps and registers aContentNegotiatingViewResolveras the highest-precedenceviewResolver. While initializing, CNVR collects everyViewResolverbean viagetBeansOfType, which force-creates the lazyjspViewResolver. If that happens before the SiteMesh 3BeanPostProcessorwrap (#15585) is in effect, CNVR captures the raw, non-decorating resolver and keeps rendering HTML through it — GSP layouts silently stop applying. Whether the race is hit depends on bean-creation order, so it varies with the plugin set: some apps on 8.0.0-M3 decorate fine, others lose all layouts.Diagnosis (reproduced on 8.0.0-M3)
In an affected app:
jspViewResolverbean =GrailsSiteMeshViewResolver@55871443wrappingScaffoldingViewResolver@344207224— the wrap itself worked.ScaffoldingViewResolver@344207224— the same instance as the wrapper's inner, captured pre-wrap.Accept: text/htmlrendered undecorated (CNVR short-circuits); requests with an Accept header CNVR cannot satisfy fell through to the wrapped resolver and rendered fully decorated.Fix
Sitemesh3ViewResolverDefinitionPostProcessor(registered bySitemesh3AutoConfiguration) rewrites thejspViewResolverbean definition into aGrailsSiteMeshViewResolverwith the original definition embedded as the inner bean — mirroring the SiteMesh 2 module'sGrailsLayoutViewResolverPostProcessor. The decorating resolver is what gets instantiated no matter how early a consumer forces the bean into existence, so the race is structurally impossible. It skips contexts where decoration is impossible (nocontentProcessor/decoratorSelector, e.g. grails-testing-support unit-test contexts) and definitions that already decorate (the SiteMesh 2 layout resolver, or an existingSiteMeshViewResolver).SiteMeshViewResolver, so the two mechanisms cannot double-decorate.Modernization (builds on #15934)
Sitemesh3GrailsPluginno longer declaresdoWithSpring()at all:Sitemesh3EnvironmentPostProcessor(META-INF/spring.factories), contributed with lowest precedence before refresh — which also removes the post-hocPropertySourcesConfigreassignment the plugin used to perform.grailsRenderViewMutatoris registered throughbeanRegistrar(), making this the first in-tree plugin on the modern registration API.Testing
Sitemesh3ViewResolverDefinitionPostProcessorSpec(9 tests) including a regression test that simulates CNVR's earlygetBeansOfType(ViewResolver)sweep against a registry with no bean post-processors at all and asserts the captured instance decorates.Sitemesh3EnvironmentPostProcessorSpec(7 tests) covering defaults, application-set keys winning, and the SiteMesh 2 default-layout fallback.:grails-sitemesh3:testand:grails-layout:testpass.grails-sitemesh3jar swapped in, both a(view:)URL mapping and a controller-rendered page produce fully decorated output, byte-identical through the CNVR path and the fall-through path.