jdaugherty commented on code in PR #15966: URL: https://github.com/apache/grails-core/pull/15966#discussion_r3562539534
########## grails-scaffolding/src/main/groovy/grails/plugin/scaffolding/ScaffoldingViewResolverDefinitionPostProcessor.java: ########## @@ -0,0 +1,89 @@ +/* + * 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 grails.plugin.scaffolding; + +import org.springframework.beans.BeansException; +import org.springframework.beans.MutablePropertyValues; +import org.springframework.beans.factory.support.BeanDefinitionRegistry; +import org.springframework.beans.factory.support.BeanDefinitionRegistryPostProcessor; +import org.springframework.beans.factory.support.GenericBeanDefinition; +import org.springframework.context.EnvironmentAware; +import org.springframework.core.Ordered; +import org.springframework.core.env.Environment; + +import grails.util.Metadata; +import org.grails.plugins.web.GroovyPagesPostProcessor; + +/** + * Registers the {@code jspViewResolver} bean definition as a + * {@link ScaffoldingViewResolver} unless a definition already exists, replacing + * the registration the plugin previously performed through the + * {@code doWithSpring()} bean DSL. Registering a definition (with the same + * {@code abstractViewResolver} parent the GSP plugin's default uses) rather + * than building the resolver directly keeps the view-resolver configuration in + * one place and preserves the established post-processor pipeline. + */ +public class ScaffoldingViewResolverDefinitionPostProcessor implements BeanDefinitionRegistryPostProcessor, EnvironmentAware, Ordered { + + /** + * Runs before the SiteMesh 2 module's {@code GrailsLayoutViewResolverPostProcessor} + * ({@code GroovyPagesPostProcessor.ORDER - 1}), which embeds the definition + * registered here as its inner view resolver, and before + * {@link GroovyPagesPostProcessor} itself, which contributes the plain GSP + * resolver only when no definition exists by then. + */ + public static final int ORDER = GroovyPagesPostProcessor.ORDER - 2; + + private static final String JSP_VIEW_RESOLVER_BEAN_NAME = "jspViewResolver"; + + private Environment environment; + + @Override + public void postProcessBeanDefinitionRegistry(BeanDefinitionRegistry registry) throws BeansException { + if (registry.containsBeanDefinition(JSP_VIEW_RESOLVER_BEAN_NAME)) { + // an application- or plugin-supplied view resolver wins Review Comment: The plugin-vs-plugin direction of this precedence actually flips relative to the old DSL registration, so "matching the old override semantics" isn't quite exact. Previously the definition was registered during the `doWithSpring()` drain in plugin load order: scaffolding (`loadAfter = ['groovyPages']`) *overwrote* a `jspViewResolver` registered by any plugin loading before it, and was itself overwritten by plugins loading after it and by the application's `resources.groovy`. Now any definition present after the drain — including one from a plugin that loads *before* scaffolding — suppresses this registration. Application precedence is preserved, and stand-down-if-present is arguably the better contract (it's what `GroovyPagesPostProcessor` does), so this is probably fine — just confirming the flip is intentional, and the inline comment could say "any existing definition wins" rather than claiming parity with the old override semantics. ########## grails-scaffolding/src/test/groovy/grails/plugin/scaffolding/ScaffoldingViewResolverDefinitionPostProcessorSpec.groovy: ########## @@ -0,0 +1,85 @@ +/* + * 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 grails.plugin.scaffolding + +import org.springframework.beans.factory.config.BeanDefinition +import org.springframework.beans.factory.support.DefaultListableBeanFactory +import org.springframework.beans.factory.support.GenericBeanDefinition +import org.springframework.core.env.MapPropertySource +import org.springframework.core.env.StandardEnvironment +import org.springframework.web.servlet.view.InternalResourceViewResolver + +import spock.lang.Specification + +class ScaffoldingViewResolverDefinitionPostProcessorSpec extends Specification { + + ScaffoldingViewResolverDefinitionPostProcessor postProcessor = new ScaffoldingViewResolverDefinitionPostProcessor() + + void "registers the scaffolding view resolver definition with the GSP parent template"() { + given: + DefaultListableBeanFactory registry = new DefaultListableBeanFactory() + postProcessor.environment = new StandardEnvironment() + + when: + postProcessor.postProcessBeanDefinitionRegistry(registry) + BeanDefinition definition = registry.getBeanDefinition('jspViewResolver') + + then: + definition.beanClassName == ScaffoldingViewResolver.name + definition.parentName == 'abstractViewResolver' + definition.lazyInit + definition.propertyValues.contains('enableReload') + definition.propertyValues.getPropertyValue('enableNamespaceViewDefaults').value == false + } + + void "the enableNamespaceViewDefaults property is read from the environment"() { + given: + DefaultListableBeanFactory registry = new DefaultListableBeanFactory() + StandardEnvironment environment = new StandardEnvironment() + environment.propertySources.addFirst( + new MapPropertySource('test', ['grails.scaffolding.enableNamespaceViewDefaults': 'true'])) + postProcessor.environment = environment + + when: + postProcessor.postProcessBeanDefinitionRegistry(registry) + + then: + registry.getBeanDefinition('jspViewResolver') + .propertyValues.getPropertyValue('enableNamespaceViewDefaults').value == true + } + + void "an existing jspViewResolver definition wins"() { + given: + DefaultListableBeanFactory registry = new DefaultListableBeanFactory() + registry.registerBeanDefinition('jspViewResolver', + new GenericBeanDefinition(beanClass: InternalResourceViewResolver)) + postProcessor.environment = new StandardEnvironment() + + when: + postProcessor.postProcessBeanDefinitionRegistry(registry) + + then: + registry.getBeanDefinition('jspViewResolver').beanClassName == InternalResourceViewResolver.name + } + + void "runs before the SiteMesh 2 layout post-processor and the GSP default post-processor"() { + expect: "GrailsLayoutViewResolverPostProcessor runs at -1 and GroovyPagesPostProcessor at 0" + postProcessor.order < -1 Review Comment: This assertion hardcodes the collaborators' orders (-1 and 0) in a comment rather than checking them, so if `GrailsLayoutViewResolverPostProcessor` or `GroovyPagesPostProcessor` were ever renumbered the test would keep passing while the pipeline contract silently broke. Asserting against the real constants pins the contract: ```groovy postProcessor.order < new GrailsLayoutViewResolverPostProcessor().order postProcessor.order < GroovyPagesPostProcessor.ORDER ``` (the second works with the existing `implementation project(':grails-gsp')`; the first needs a `testImplementation project(':grails-layout')` — if that dependency isn't wanted, at least the `GroovyPagesPostProcessor.ORDER` half is free). -- 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]
