jdaugherty commented on code in PR #16709: URL: https://github.com/apache/grails-core/pull/16709#discussion_r4338966792
########## grails-core/src/main/groovy/org/apache/grails/core/aot/GeneratedBeanDefinitions.java: ########## @@ -0,0 +1,107 @@ +/* + * 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.apache.grails.core.aot; + +import java.util.IdentityHashMap; +import java.util.Map; + +import org.jspecify.annotations.Nullable; + +import org.springframework.beans.factory.BeanFactory; +import org.springframework.beans.factory.config.BeanDefinition; +import org.springframework.beans.factory.config.ConfigurableListableBeanFactory; +import org.springframework.util.ClassUtils; + +/** + * The bean definitions the code generated ahead of time contributed, recorded before anything else is + * declared, with the type each one declares. + * + * <p>An application running on generated artifacts has the processors that inject through annotations + * registered again, for the beans the generated code could not see into. The generated code injects a bean + * through the annotated members of the type its definition declares, and records that type's initialization + * and destruction methods, so those processors leave a bean of exactly that type alone; one injected twice + * runs a side-effecting {@code @Autowired} method twice, which registered every executor's metrics a second + * time.</p> + * + * <p>A bean of any other class has members the generated code never saw: a factory method declaring an Review Comment: This is exact for an interface, not for a declared superclass. The generated code injects the members of the declared type: both `AutowiredAnnotationBeanPostProcessor.processAheadOfTime` and `InitDestroyAnnotationBeanPostProcessor.processAheadOfTime` work from `RegisteredBean.getBeanClass()`, the user class of that type. So for a factory method declared as an abstract class with an `@Autowired` member that returns a subclass, the generated supplier injects the base member, and the processor, seeing a class other than the declared one, injects the whole class again: the base's `@Autowired` method runs twice. `@PostConstruct` is not affected, because the processor registers it as externally managed and the container then skips the recorded init method name. Checked with a fixture on this branch: a definition declared as `Base` (an `@Autowired` setter and a `@PostConstruct` method), a supplier returning `new Sub()` with the setter already called, `initMethodNames = ['initialize']`; after `refresh()`, `injections == 2` and `initializations == 1`. The processor has no way to inject only the members below the declared type, so I'm not asking for different behaviour here. Please say in this paragraph that a bean declared as a superclass is still injected twice through that superclass's members, so the next report of a doubled `@Autowired` method is read against the right rule. ########## grails-core/src/main/groovy/grails/boot/config/GrailsPluginLifecycleInitializer.java: ########## @@ -29,12 +32,19 @@ * {@code doWithSpring} be present in the registry before Boot's {@code @ConditionalOnMissingBean} * guards are evaluated, so auto-configured defaults back off in favour of plugin beans. * + * <p>On generated artifacts it also records the bean definitions the generated code contributed. Spring Boot + * applies the generated initializer before any other, and no plugin has declared a bean yet, so what the Review Comment: "Before any other" is more than Boot does. `SpringApplication.addAotGeneratedInitializerIfNecessary` puts the generated initializer at index 0, but `getInitializers()` sorts with `AnnotationAwareOrderComparator`, so an initializer with an order runs before it; in Boot 4.1.1 that is `ParentContextApplicationContextInitializer` (highest precedence) and `ContextIdApplicationContextInitializer` (lowest minus 10). The sort is stable, so the generated one does run before this one and before anything added through the builder, which are unordered. What the record relies on is that nothing running before the generated initializer registers a bean definition. Boot's two set the parent and register a singleton, so it holds; an application initializer with an order that registers a definition itself would have it recorded as generated and, when its class is the one it declares, injected by nobody. Worth stating that condition here rather than the stronger one. ########## grails-core/src/main/groovy/grails/boot/config/GrailsApplicationPostProcessor.groovy: ########## @@ -344,23 +346,33 @@ class GrailsApplicationPostProcessor implements BeanDefinitionRegistryPostProces * * <p>They are registered under the names Spring uses itself, so a context that already has them * keeps what it has, and a context running without generated artifacts is untouched.</p> + * + * <p>They leave alone the beans the generated code created, which it has already injected: injected a Review Comment: The guide still says the opposite: "What AOT changes at runtime" in `grails-doc/src/en/guide/deployment/deploymentAot.adoc` lists `internalAutowiredAnnotationProcessor` and `internalCommonAnnotationProcessor` among the infrastructure an AOT-processed application no longer has. They have been back under those names since b1611e36ab, and with this change they are the variants that stand down for generated beans. Please bring that paragraph up to date in this PR: the two injection processors are registered again for the beans plugins declare as the application starts, and a generated bean is injected once, by the generated code. ########## grails-core/src/main/groovy/org/apache/grails/core/aot/GeneratedArtifactsCommonAnnotationBeanPostProcessor.java: ########## @@ -0,0 +1,79 @@ +/* + * 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.apache.grails.core.aot; + +import org.jspecify.annotations.Nullable; + +import org.springframework.beans.BeansException; +import org.springframework.beans.PropertyValues; +import org.springframework.beans.factory.BeanFactory; +import org.springframework.beans.factory.support.RootBeanDefinition; +import org.springframework.context.annotation.CommonAnnotationBeanPostProcessor; + +/** + * Injects through {@code @Resource}, and calls {@code @PostConstruct} and {@code @PreDestroy} methods on, the + * beans the code generated ahead of time did not handle, and leaves alone the ones it did -- the beans it + * created that are of the type their definitions declare: it has injected those and recorded their + * initialization and destruction methods on their definitions. + * + * @since 8.0.1 + * @see GeneratedBeanDefinitions + */ +public class GeneratedArtifactsCommonAnnotationBeanPostProcessor extends CommonAnnotationBeanPostProcessor { + + @Nullable + private BeanFactory beanFactory; + + @Override + public void setBeanFactory(BeanFactory beanFactory) { + super.setBeanFactory(beanFactory); + this.beanFactory = beanFactory; + } + + @Override + public void postProcessMergedBeanDefinition(RootBeanDefinition beanDefinition, Class<?> beanType, String beanName) { + if (!GeneratedBeanDefinitions.isInjectedByGeneratedCode(beanFactory, beanName, beanType)) { + super.postProcessMergedBeanDefinition(beanDefinition, beanType, beanName); + } + } + + @Override + public PropertyValues postProcessProperties(PropertyValues pvs, Object bean, String beanName) throws BeansException { + if (GeneratedBeanDefinitions.isInjectedByGeneratedCode(beanFactory, beanName, bean.getClass())) { + return pvs; + } + return super.postProcessProperties(pvs, bean, beanName); + } + + @Override + public Object postProcessBeforeInitialization(Object bean, String beanName) throws BeansException { + if (GeneratedBeanDefinitions.isInjectedByGeneratedCode(beanFactory, beanName, bean.getClass())) { + return bean; + } + return super.postProcessBeforeInitialization(bean, beanName); + } + + @Override + public void postProcessBeforeDestruction(Object bean, String beanName) throws BeansException { Review Comment: Nothing exercises this override; it is the uncovered part of this class in the coverage report. `GrailsApplicationPostProcessorAnnotationConfigSpec` could give `InjectedFixture` a `@PreDestroy` method, set `destroyMethodNames` on the generated definition as the generated code does, and assert after `context.close()` that it ran once: `DisposableBeanAdapter` invokes the recorded name when this processor stands down. I ran that shape locally and it passes, so this is coverage of the claim in the class javadoc, not a bug. -- 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]
