jdaugherty commented on code in PR #15964: URL: https://github.com/apache/grails-core/pull/15964#discussion_r3562457326
########## grails-gsp/grails-sitemesh3/src/main/java/org/grails/plugins/sitemesh3/Sitemesh3EnvironmentPostProcessor.java: ########## @@ -0,0 +1,92 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.grails.plugins.sitemesh3; + +import java.util.LinkedHashMap; +import java.util.Map; + +import org.springframework.boot.SpringApplication; +import org.springframework.boot.env.EnvironmentPostProcessor; +import org.springframework.core.Ordered; +import org.springframework.core.env.ConfigurableEnvironment; +import org.springframework.core.env.MapPropertySource; + +import org.grails.web.util.WebUtils; + +/** + * Contributes the Grails defaults for the SiteMesh 3 configuration keys — + * layout selection via the {@code layout} meta tag, the + * {@code /layouts/} decorator prefix and, when configured, the application's + * default layout — before the application context refreshes. + * + * <p>Because these defaults are in the {@link ConfigurableEnvironment} from the + * start, both the SiteMesh starter's {@code @Value} placeholders and the Grails + * configuration (which is built from the environment) observe them without any + * post-hoc reassignment. This replaces the property-source manipulation the + * plugin previously performed in {@code doWithSpring()}.</p> + * + * <p>Each default is contributed only when the application has not set the key + * itself, and the source is appended with lowest precedence, so application + * configuration always wins.</p> + */ +public class Sitemesh3EnvironmentPostProcessor implements EnvironmentPostProcessor, Ordered { Review Comment: Why not use groovy? ########## grails-gsp/grails-sitemesh3/src/main/java/org/grails/plugins/sitemesh3/Sitemesh3ViewResolverDefinitionPostProcessor.java: ########## @@ -0,0 +1,142 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.grails.plugins.sitemesh3; + +import org.sitemesh.webmvc.SiteMeshViewResolver; +import org.sitemesh.webmvc.SiteMeshViewResolverPostProcessor; + +import org.springframework.beans.BeansException; +import org.springframework.beans.factory.annotation.AnnotatedBeanDefinition; +import org.springframework.beans.factory.config.BeanDefinition; +import org.springframework.beans.factory.config.ConstructorArgumentValues; +import org.springframework.beans.factory.config.RuntimeBeanReference; +import org.springframework.beans.factory.support.BeanDefinitionRegistry; +import org.springframework.beans.factory.support.GenericBeanDefinition; +import org.springframework.context.ApplicationListener; +import org.springframework.core.type.MethodMetadata; +import org.springframework.util.ClassUtils; + +import org.grails.plugins.web.GroovyPagesPostProcessor; + +/** + * Grails-flavoured {@link SiteMeshViewResolverPostProcessor} — the upstream + * bean-definition wrap mode ({@code sitemesh.viewResolver.wrapMode=bean-definition}) + * expressed with Grails semantics. It rewrites the {@code jspViewResolver} bean + * definition into a {@link GrailsSiteMeshViewResolver} definition, so that every + * instantiation of the bean — however early — yields the decorating resolver. + * + * <p>Wrapping at the bean-definition level (rather than post-processing the bean + * instance) closes an initialization-order race: {@code jspViewResolver} is + * registered lazy, so it is instantiated by whichever component first asks for + * it. If that consumer is initialized before the SiteMesh + * {@code BeanPostProcessor} takes effect — Spring Boot's + * {@code ContentNegotiatingViewResolver} collecting every {@code ViewResolver} + * while it initializes is one such consumer — it captures the raw, undecorating + * resolver and keeps rendering through it, silently disabling layouts. This + * mirrors the approach the SiteMesh 2 module takes with its + * {@code GrailsLayoutViewResolverPostProcessor}.</p> + * + * <p>It deliberately diverges from the upstream implementation on one point: + * upstream re-registers the unwrapped resolver as a separate named bean + * ({@code innerBeanName}) that the wrapper references, which leaves the raw + * resolver discoverable by {@code getBeansOfType(ViewResolver)} sweeps — the + * exact exposure this class exists to close. The original definition is instead + * embedded as an anonymous inner-bean definition of the wrapper, making the + * undecorated resolver structurally unreachable.</p> + * + * <p>Runs after {@link GroovyPagesPostProcessor} (which contributes the default + * GSP resolver definition when no plugin has registered one) so the definition + * being wrapped is final, whether it came from grails-gsp, the scaffolding + * plugin, or the application.</p> + */ +public class Sitemesh3ViewResolverDefinitionPostProcessor extends SiteMeshViewResolverPostProcessor { + + /** + * After {@link GroovyPagesPostProcessor#ORDER} so the default GSP resolver + * definition exists, and after the SiteMesh 2 module's post-processor + * (ORDER - 1) so legacy layout wrapping, when present, wins and is detected. + */ + public static final int ORDER = GroovyPagesPostProcessor.ORDER + 10; + + public Sitemesh3ViewResolverDefinitionPostProcessor() { + setTargetViewResolverBeanName(GrailsSiteMeshViewResolverBeanPostProcessor.TARGET_VIEW_RESOLVER_BEAN_NAME); + setSiteMeshViewResolverClass(GrailsSiteMeshViewResolver.class); + setOrder(ORDER); + } + + @Override + public void postProcessBeanDefinitionRegistry(BeanDefinitionRegistry registry) throws BeansException { + if (!registry.containsBeanDefinition(getTargetViewResolverBeanName()) || + !registry.containsBeanDefinition(getContentProcessorBeanName()) || + !registry.containsBeanDefinition(getDecoratorSelectorBeanName())) { + // Decoration is not possible in this context (no GSP view resolver, or a + // context without the SiteMesh beans, e.g. the lightweight unit-test + // contexts built by grails-testing-support) — leave the definition alone. + return; + } + BeanDefinition existing = registry.getBeanDefinition(getTargetViewResolverBeanName()); + if (isAlreadyDecorating(existing)) { + return; + } + registry.removeBeanDefinition(getTargetViewResolverBeanName()); + + GenericBeanDefinition wrapper = new GenericBeanDefinition(); + wrapper.setBeanClass(getSiteMeshViewResolverClass()); + wrapper.setLazyInit(existing.isLazyInit()); + wrapper.setPrimary(true); + ConstructorArgumentValues arguments = wrapper.getConstructorArgumentValues(); + arguments.addIndexedArgumentValue(0, existing); + arguments.addIndexedArgumentValue(1, new RuntimeBeanReference(getContentProcessorBeanName())); + arguments.addIndexedArgumentValue(2, new RuntimeBeanReference(getDecoratorSelectorBeanName())); + arguments.addIndexedArgumentValue(3, new RuntimeBeanReference(getServletContextBeanName())); + if (getDispatchMode() != null) { + wrapper.getPropertyValues().add("dispatchMode", getDispatchMode()); + } + wrapper.getPropertyValues().add("includeErrorPages", isIncludeErrorPages()); + registry.registerBeanDefinition(getTargetViewResolverBeanName(), wrapper); + } + + /** + * Skips definitions that already decorate: a {@link SiteMeshViewResolver} + * (this module's wrapper, or a custom one), or the legacy grails-layout + * module's {@code GrailsLayoutViewResolver} — an {@link ApplicationListener} + * that performs SiteMesh 2 decoration itself, matching the instance-level + * exclusion {@link GrailsSiteMeshViewResolverBeanPostProcessor} applies. + */ + private boolean isAlreadyDecorating(BeanDefinition definition) { + String className = definition.getBeanClassName(); + if (className == null && definition instanceof AnnotatedBeanDefinition annotated) { + MethodMetadata factoryMethod = annotated.getFactoryMethodMetadata(); + if (factoryMethod != null) { + className = factoryMethod.getReturnTypeName(); + } + } + if (className == null) { + return false; + } + try { + Class<?> beanClass = ClassUtils.forName(className, getClass().getClassLoader()); Review Comment: `getClass().getClassLoader()` is the classloader that loaded the grails-sitemesh3 jar, so it can miss application-defined classes when app classes live in a child/restart classloader (e.g. devtools dev-mode reloading). A user-defined `SiteMeshViewResolver` subclass registered as `jspViewResolver` would then fail `forName`, be treated as non-decorating, and get double-wrapped. The check should resolve with the same loader the container will use when it instantiates the definition — the bean factory's bean classloader (`AbstractBeanDefinition.resolveBeanClass` uses it), falling back to the thread context classloader: ```java ClassLoader loader = registry instanceof ConfigurableBeanFactory cbf ? cbf.getBeanClassLoader() : ClassUtils.getDefaultClassLoader(); // TCCL with fallbacks Class<?> beanClass = ClassUtils.forName(className, loader); ``` (the registry passed to `postProcessBeanDefinitionRegistry` is the `DefaultListableBeanFactory` itself in practice, and `getBeanClassLoader()` defaults to `ClassUtils.getDefaultClassLoader()` anyway, so both branches agree unless a custom bean classloader was set). This requires threading the registry into `isAlreadyDecorating`. ########## grails-gsp/grails-sitemesh3/src/main/groovy/org/grails/plugins/sitemesh3/Sitemesh3GrailsPlugin.groovy: ########## @@ -49,39 +66,10 @@ class Sitemesh3GrailsPlugin extends Plugin { Sitemesh3LayoutTagLib, ] - static PropertySource getDefaultPropertySource(ConfigurableEnvironment configurableEnvironment, String defaultLayout) { - Map props = [ - 'sitemesh.decorator.metaTag': 'layout', - 'sitemesh.decorator.attribute': WebUtils.LAYOUT_ATTRIBUTE, - 'sitemesh.decorator.prefix': '/layouts/', - ] - if (defaultLayout) { - props['sitemesh.decorator.default'] = defaultLayout - } - props.clone().each { - if (configurableEnvironment.getProperty(it.key)) { - props.remove(it.key) - } - } - new MapPropertySource('defaultSitemesh3Properties', props) - } - - Closure doWithSpring() { - { -> - ConfigurableEnvironment configurableEnvironment = grailsApplication.mainContext.environment as ConfigurableEnvironment - def propertySources = configurableEnvironment.getPropertySources() - // The SiteMesh 3 specific key wins; fall back to the SiteMesh 2 - // plugin's grails.views.layout.default so existing apps keep - // their configured default layout when switching. - String defaultLayout = grailsApplication.getConfig().getProperty('grails.sitemesh.default.layout') ?: - grailsApplication.getConfig().getProperty('grails.views.layout.default') - propertySources.addFirst(getDefaultPropertySource(configurableEnvironment, defaultLayout)) - (grailsApplication as DefaultGrailsApplication).config = new PropertySourcesConfig(propertySources) - - // Unwraps the SiteMesh view for "render template:" partials so - // they are never decorated with a layout (the SiteMesh 2 plugin - // does the same with its GrailsLayoutRenderViewMutator). - grailsRenderViewMutator(Sitemesh3RenderViewMutator) + @Override + BeanRegistrar beanRegistrar() { + return { BeanRegistry registry, Environment environment -> + registry.registerBean('grailsRenderViewMutator', Sitemesh3RenderViewMutator) Review Comment: The class javadoc states that installing this plugin alongside grails-layout "is supported; in that case the SiteMesh 2 integration keeps decorating and this one stands down" — but SiteMesh 2 and SiteMesh 3 are never meant to be installed together (grails-layout only exists when sitemesh3 is missing). The javadoc should say the two are mutually exclusive rather than promise coexistence semantics. Worth noting because the stand-down promise is no longer fully true anyway: registrar beans are applied after the `doWithSpring()` drain and win name conflicts (see `GrailsEarlyPluginRegistrationPostProcessor.applyBeanRegistrars`), so if both modules ever do end up on the classpath, this `grailsRenderViewMutator` now displaces SM2's `GrailsLayoutRenderViewMutator` (which previously won via plugin order) and SM2 layout views would no longer be unwrapped for `render template:` partials. If mutual exclusion is the contract, consider having the plugin fail fast (or at least warn) when grails-layout is detected, instead of documenting cooperative behavior. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
