matrei commented on code in PR #15994:
URL: https://github.com/apache/grails-core/pull/15994#discussion_r3608879240
##########
grails-async/plugin/src/main/groovy/org/grails/plugins/web/async/ControllersAsyncGrailsPlugin.groovy:
##########
@@ -29,14 +35,17 @@ import
org.grails.plugins.web.async.spring.PromiseFactoryBean
* @author Graeme Rocher
* @since 2.0
*/
+@CompileStatic
class ControllersAsyncGrailsPlugin extends Plugin {
def grailsVersion = '7.0.0-SNAPSHOT > *'
Review Comment:
Should we update all these to `8.0.0-SNAPSHOT > *`?
##########
grails-cache/src/main/groovy/grails/plugin/cache/GrailsCacheAutoConfiguration.groovy:
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.cache
+
+import groovy.transform.CompileStatic
+
+import org.springframework.beans.factory.annotation.Value
+import org.springframework.boot.autoconfigure.AutoConfiguration
+import org.springframework.boot.autoconfigure.condition.ConditionalOnBean
+import
org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean
+import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty
+import org.springframework.context.annotation.Bean
+
+import org.grails.plugin.cache.GrailsCacheManager
+
+/**
+ * Auto-configures the cache plugin's default cache manager and key generator.
Registered here
+ * rather than by the plugin descriptor so that a bean contributed by the
application or another
+ * plugin — for example a cache-provider plugin's {@code grailsCacheManager} —
makes the default
+ * back off cleanly instead of triggering a bean-definition override.
+ *
+ * <p>Gated on the {@code CachePluginConfiguration} definition contributed by
the cache plugin
+ * descriptor's registrar (which runs before auto-configuration conditions are
evaluated), so the
+ * auto-configuration backs off entirely when the plugin is not active — e.g.
the jar is on the
+ * classpath but the plugin is excluded — keeping it in lockstep with the
descriptor.</p>
+ *
+ * @since 8.0
+ */
+@AutoConfiguration
+@ConditionalOnProperty(name = 'grails.cache.enabled', matchIfMissing = true)
Review Comment:
```suggestion
@ConditionalOnBooleanProperty(name = 'grails.cache.enabled', matchIfMissing
= true)
```
##########
grails-cache/src/main/groovy/grails/plugin/cache/GrailsCacheAutoConfiguration.groovy:
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.cache
+
+import groovy.transform.CompileStatic
+
+import org.springframework.beans.factory.annotation.Value
+import org.springframework.boot.autoconfigure.AutoConfiguration
+import org.springframework.boot.autoconfigure.condition.ConditionalOnBean
+import
org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean
+import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty
+import org.springframework.context.annotation.Bean
+
+import org.grails.plugin.cache.GrailsCacheManager
+
+/**
+ * Auto-configures the cache plugin's default cache manager and key generator.
Registered here
+ * rather than by the plugin descriptor so that a bean contributed by the
application or another
+ * plugin — for example a cache-provider plugin's {@code grailsCacheManager} —
makes the default
+ * back off cleanly instead of triggering a bean-definition override.
+ *
+ * <p>Gated on the {@code CachePluginConfiguration} definition contributed by
the cache plugin
+ * descriptor's registrar (which runs before auto-configuration conditions are
evaluated), so the
+ * auto-configuration backs off entirely when the plugin is not active — e.g.
the jar is on the
+ * classpath but the plugin is excluded — keeping it in lockstep with the
descriptor.</p>
+ *
+ * @since 8.0
+ */
+@AutoConfiguration
+@ConditionalOnProperty(name = 'grails.cache.enabled', matchIfMissing = true)
+@ConditionalOnBean(CachePluginConfiguration)
+@CompileStatic
+class GrailsCacheAutoConfiguration {
+
+ @Value('${grails.cache.cacheManager:}')
+ String cacheManagerType
+
+ @Bean
+ @ConditionalOnMissingBean(name = 'customCacheKeyGenerator')
+ CustomCacheKeyGenerator customCacheKeyGenerator() {
+ new CustomCacheKeyGenerator()
+ }
+
+ @Bean
+ @ConditionalOnMissingBean(name = 'grailsCacheManager')
+ GrailsCacheManager grailsCacheManager(CachePluginConfiguration
grailsCacheConfiguration) {
+ if (cacheManagerType == 'GrailsConcurrentLinkedMapCacheManager') {
+ GrailsConcurrentLinkedMapCacheManager cacheManager = new
GrailsConcurrentLinkedMapCacheManager()
+ cacheManager.configuration = grailsCacheConfiguration
+ return cacheManager
+ }
+ GrailsConcurrentMapCacheManager cacheManager = new
GrailsConcurrentMapCacheManager()
+ cacheManager.configuration = grailsCacheConfiguration
+ return cacheManager
Review Comment:
```suggestion
return new GrailsConcurrentLinkedMapCacheManager(configuration:
grailsCacheConfiguration)
}
new GrailsConcurrentMapCacheManager(configuration:
grailsCacheConfiguration)
```
##########
grails-cache/src/test/groovy/grails/plugin/cache/CacheGrailsPluginSpec.groovy:
##########
@@ -0,0 +1,74 @@
+/*
+ * 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.cache
+
+import org.springframework.beans.factory.BeanRegistrar
+import org.springframework.beans.factory.support.BeanRegistryAdapter
+import org.springframework.beans.factory.support.DefaultListableBeanFactory
+import org.springframework.core.env.MapPropertySource
+import org.springframework.core.env.StandardEnvironment
+
+import spock.lang.Specification
+
+class CacheGrailsPluginSpec extends Specification {
+
+ void "beanRegistrar registers the cache infrastructure beans"() {
+ given:
+ DefaultListableBeanFactory beanFactory = new
DefaultListableBeanFactory()
Review Comment:
For this line (and all other occurrences where the type can be inferred) the
readability is really improved by using `def` (or `var` for Java).
##########
grails-converters/src/main/groovy/org/grails/plugins/converters/ConvertersGrailsPlugin.groovy:
##########
@@ -48,22 +55,30 @@ class ConvertersGrailsPlugin extends Plugin {
]
@Override
- Closure doWithSpring() {
- { ->
- jsonErrorsMarshaller(JsonErrorsMarshaller)
+ BeanRegistrar beanRegistrar() {
+ return { BeanRegistry registry, Environment environment ->
+ registry.registerBean('jsonErrorsMarshaller', JsonErrorsMarshaller)
- xmlErrorsMarshaller(XmlErrorsMarshaller)
+ registry.registerBean('xmlErrorsMarshaller', XmlErrorsMarshaller)
-
convertersConfigurationInitializer(ConvertersConfigurationInitializer)
+ registry.registerBean('convertersConfigurationInitializer',
ConvertersConfigurationInitializer)
- errorsXmlMarshallerRegisterer(ObjectMarshallerRegisterer) {
- marshaller = { XmlErrorsMarshaller om -> }
- converterClass = XML
+ registry.registerBean('errorsXmlMarshallerRegisterer',
ObjectMarshallerRegisterer) { BeanRegistry.Spec<ObjectMarshallerRegisterer>
spec ->
+ spec.supplier { BeanRegistry.SupplierContext context ->
+ ObjectMarshallerRegisterer registerer = new
ObjectMarshallerRegisterer()
+ registerer.marshaller =
context.bean('xmlErrorsMarshaller', XmlErrorsMarshaller)
+ registerer.converterClass = XML
+ return registerer
+ }
}
- errorsJsonMarshallerRegisterer(ObjectMarshallerRegisterer) {
- marshaller = { JsonErrorsMarshaller om -> }
- converterClass = JSON
+ registry.registerBean('errorsJsonMarshallerRegisterer',
ObjectMarshallerRegisterer) { BeanRegistry.Spec<ObjectMarshallerRegisterer>
spec ->
+ spec.supplier { BeanRegistry.SupplierContext context ->
+ ObjectMarshallerRegisterer registerer = new
ObjectMarshallerRegisterer()
+ registerer.marshaller =
context.bean('jsonErrorsMarshaller', JsonErrorsMarshaller)
+ registerer.converterClass = JSON
+ return registerer
+ }
}
Review Comment:
```suggestion
registry.registerBean('errorsXmlMarshallerRegisterer',
ObjectMarshallerRegisterer) {
it.supplier {
new ObjectMarshallerRegisterer(
marshaller: it.bean('xmlErrorsMarshaller',
XmlErrorsMarshaller),
converterClass: XML
)
}
}
registry.registerBean('errorsJsonMarshallerRegisterer',
ObjectMarshallerRegisterer) {
it.supplier {
new ObjectMarshallerRegisterer(
marshaller: it.bean('jsonErrorsMarshaller',
JsonErrorsMarshaller),
converterClass: JSON
)
}
}
```
##########
grails-controllers/src/main/groovy/org/grails/plugins/web/controllers/ControllerBeanDefinitionsPostProcessor.groovy:
##########
@@ -0,0 +1,94 @@
+/*
+ * 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.web.controllers
+
+import groovy.transform.CompileStatic
+import groovy.util.logging.Slf4j
+
+import org.springframework.beans.BeansException
+import org.springframework.beans.factory.support.AbstractBeanDefinition
+import org.springframework.beans.factory.support.BeanDefinitionRegistry
+import
org.springframework.beans.factory.support.BeanDefinitionRegistryPostProcessor
+import org.springframework.beans.factory.support.GenericBeanDefinition
+import org.springframework.core.Ordered
+import org.springframework.core.PriorityOrdered
+
+import grails.core.GrailsApplication
+import grails.core.GrailsClass
+import grails.core.GrailsControllerClass
+import org.grails.core.artefact.ControllerArtefactHandler
+
+/**
+ * Registers a bean definition for every controller artefact, replacing the
registration the
+ * controllers plugin previously performed through the {@code doWithSpring()}
bean DSL. Controller
+ * beans autowire by name, use the scope declared on the controller class and
cannot be expressed
+ * through the {@link org.springframework.beans.factory.BeanRegistry} API, so
the definitions are
+ * contributed by this post-processor instead.
+ *
+ * <p>Runs as a {@link PriorityOrdered} post-processor with highest precedence
so the controller
+ * definitions are registered before Spring Boot's configuration-class
post-processor evaluates
+ * auto-configuration conditions — the same visibility the {@code
doWithSpring()} registration had.
+ * An existing definition for a controller name wins, preserving the ability
of the application
+ * (or another plugin) to override a controller bean.</p>
+ *
+ * @since 8.0
+ */
+@Slf4j
+@CompileStatic
+class ControllerBeanDefinitionsPostProcessor implements
BeanDefinitionRegistryPostProcessor, PriorityOrdered {
+
+ private final GrailsApplication grailsApplication
+ private final boolean useJsessionId
+
+ ControllerBeanDefinitionsPostProcessor(GrailsApplication
grailsApplication, boolean useJsessionId) {
+ this.grailsApplication = grailsApplication
+ this.useJsessionId = useJsessionId
+ }
+
+ @Override
+ void postProcessBeanDefinitionRegistry(BeanDefinitionRegistry registry)
throws BeansException {
+ for (GrailsClass controller in
grailsApplication.getArtefacts(ControllerArtefactHandler.TYPE)) {
+ log.debug('Configuring controller {}', controller.fullName)
+ GrailsControllerClass controllerClass = (GrailsControllerClass)
controller
+ if (!controllerClass.available ||
registry.containsBeanDefinition(controllerClass.fullName)) {
+ continue
+ }
+ Object lazyInit = controllerClass.hasProperty('lazyInit') ?
controllerClass.getPropertyValue('lazyInit') : true
+
+ GenericBeanDefinition definition = new GenericBeanDefinition()
+ definition.beanClass = controllerClass.clazz
+ definition.lazyInit = lazyInit as boolean
+ String beanScope = controllerClass.getScope()
+ definition.scope = beanScope
+ definition.autowireMode = AbstractBeanDefinition.AUTOWIRE_BY_NAME
+ if (beanScope == 'prototype') {
+ definition.dependencyCheck =
AbstractBeanDefinition.DEPENDENCY_CHECK_NONE
+ }
+ if (useJsessionId) {
+ definition.propertyValues.addPropertyValue('useJessionId',
useJsessionId)
+ }
+ registry.registerBeanDefinition(controllerClass.fullName,
definition)
+ }
+ }
Review Comment:
```suggestion
@Override
void postProcessBeanDefinitionRegistry(BeanDefinitionRegistry registry)
throws BeansException {
for (def controllerClass :
resolveControllerClasses(grailsApplication)) {
log.debug('Configuring controller {}', controllerClass.fullName)
if (!controllerClass.available ||
registry.containsBeanDefinition(controllerClass.fullName)) {
continue
}
def beanDefinition = new GenericBeanDefinition(
beanClass: controllerClass.clazz,
lazyInit: resolveControllerLazyInit(controllerClass),
scope: controllerClass.scope,
autowireMode: AbstractBeanDefinition.AUTOWIRE_BY_NAME
)
if (controllerClass.scope == 'prototype') {
beanDefinition.dependencyCheck =
AbstractBeanDefinition.DEPENDENCY_CHECK_NONE
}
if (useJsessionId) {
beanDefinition.propertyValues.addPropertyValue('useJessionId', useJsessionId)
}
registry.registerBeanDefinition(controllerClass.fullName,
beanDefinition)
}
}
private static List<GrailsControllerClass>
resolveControllerClasses(GrailsApplication grailsApplication) {
grailsApplication.getArtefacts(ControllerArtefactHandler.TYPE).collect {
(GrailsControllerClass) it
}
}
private static boolean resolveControllerLazyInit(GrailsControllerClass
controllerClass) {
controllerClass.hasProperty('lazyInit') ?
controllerClass.getPropertyValue('lazyInit') : true
}
```
##########
grails-converters/src/test/groovy/org/grails/plugins/converters/ConvertersGrailsPluginSpec.groovy:
##########
@@ -0,0 +1,64 @@
+/*
+ * 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.converters
+
+import org.springframework.beans.factory.BeanRegistrar
+import org.springframework.beans.factory.support.BeanRegistryAdapter
+import org.springframework.beans.factory.support.DefaultListableBeanFactory
+import org.springframework.core.env.StandardEnvironment
+
+import grails.converters.JSON
+import grails.converters.XML
+import
org.grails.web.converters.configuration.ConvertersConfigurationInitializer
+import org.grails.web.converters.configuration.ObjectMarshallerRegisterer
+import org.grails.web.converters.marshaller.json.ValidationErrorsMarshaller as
JsonErrorsMarshaller
+import org.grails.web.converters.marshaller.xml.ValidationErrorsMarshaller as
XmlErrorsMarshaller
+
+import spock.lang.Specification
+
+class ConvertersGrailsPluginSpec extends Specification {
+
+ DefaultListableBeanFactory beanFactory = new DefaultListableBeanFactory()
+
+ void setup() {
+ BeanRegistrar registrar = new ConvertersGrailsPlugin().beanRegistrar()
+ new BeanRegistryAdapter(beanFactory, new StandardEnvironment(),
registrar.getClass()).register(registrar)
+ }
+
+ void "beanRegistrar registers the converters beans"() {
+ expect:
+ beanFactory.getBeanDefinition('jsonErrorsMarshaller').beanClassName ==
JsonErrorsMarshaller.name
+ beanFactory.getBeanDefinition('xmlErrorsMarshaller').beanClassName ==
XmlErrorsMarshaller.name
+
beanFactory.getBeanDefinition('convertersConfigurationInitializer').beanClassName
== ConvertersConfigurationInitializer.name
+ beanFactory.containsBeanDefinition('errorsXmlMarshallerRegisterer')
+ beanFactory.containsBeanDefinition('errorsJsonMarshallerRegisterer')
Review Comment:
Instances like these can be cleaned up with Spocks `with`-feature:
```suggestion
with(beanFactory) {
getBeanDefinition('jsonErrorsMarshaller').beanClassName ==
JsonErrorsMarshaller.name
getBeanDefinition('xmlErrorsMarshaller').beanClassName ==
XmlErrorsMarshaller.name
getBeanDefinition('convertersConfigurationInitializer').beanClassName ==
ConvertersConfigurationInitializer.name
containsBeanDefinition('errorsXmlMarshallerRegisterer')
containsBeanDefinition('errorsJsonMarshallerRegisterer')
}
```
##########
grails-controllers/src/main/groovy/org/grails/plugins/web/controllers/ControllersGrailsPlugin.groovy:
##########
@@ -51,51 +56,35 @@ class ControllersGrailsPlugin extends Plugin {
def dependsOn = [core: version, i18n: version, urlMappings: version]
@Override
- Closure doWithSpring() {
- { ->
- def application = grailsApplication
- def config = application.config
-
- boolean useJsessionId =
config.getProperty(Settings.GRAILS_VIEWS_ENABLE_JSESSIONID, Boolean, false)
+ BeanRegistrar beanRegistrar() {
+ return { BeanRegistry registry, Environment environment ->
+ boolean useJsessionId =
environment.getProperty(Settings.GRAILS_VIEWS_ENABLE_JSESSIONID, Boolean, false)
if
(!Boolean.parseBoolean(System.getProperty(Settings.SETTING_SKIP_BOOTSTRAP))) {
- bootStrapClassRunner(BootStrapClassRunner)
+ registry.registerBean('bootStrapClassRunner',
BootStrapClassRunner)
}
-
tokenResponseActionResultTransformer(TokenResponseActionResultTransformer)
-
- exceptionHandler(GrailsExceptionResolver) {
- exceptionMappings = ['java.lang.Exception': '/error']
- }
+ registry.registerBean('tokenResponseActionResultTransformer',
TokenResponseActionResultTransformer)
- "${CompositeViewResolver.BEAN_NAME}"(CompositeViewResolver)
+ registry.registerBean(CompositeViewResolver.BEAN_NAME,
CompositeViewResolver)
- for (controller in
application.getArtefacts(ControllerArtefactHandler.TYPE)) {
- log.debug('Configuring controller {}', controller.fullName)
- if (controller.available) {
- def lazyInit = controller.hasProperty('lazyInit') ?
controller.getPropertyValue('lazyInit') : true
- "${controller.fullName}"(controller.clazz) { bean ->
- bean.lazyInit = lazyInit
- def beanScope = controller.getScope()
- bean.scope = beanScope
- bean.autowire = 'byName'
- if (beanScope == 'prototype') {
- bean.beanDefinition.dependencyCheck =
AbstractBeanDefinition.DEPENDENCY_CHECK_NONE
- }
- if (useJsessionId) {
- useJessionId = useJsessionId
- }
- }
+ // Controller beans autowire by name and use per-controller
scopes, which the
+ // BeanRegistry API cannot express — their definitions are
contributed by a
+ // dedicated post-processor instead
+ registry.registerBean('controllerBeanDefinitionsPostProcessor',
ControllerBeanDefinitionsPostProcessor) {
BeanRegistry.Spec<ControllerBeanDefinitionsPostProcessor> spec ->
+ spec.infrastructure().supplier { BeanRegistry.SupplierContext
context ->
+ new
ControllerBeanDefinitionsPostProcessor(grailsApplication, useJsessionId)
}
}
Review Comment:
```suggestion
registry.registerBean('controllerBeanDefinitionsPostProcessor',
ControllerBeanDefinitionsPostProcessor) {
it.infrastructure().supplier {
new
ControllerBeanDefinitionsPostProcessor(grailsApplication, useJsessionId)
}
}
```
--
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]
