AOT registers BeanDefinitionRegistryPostProcessor beans causing duplication of configuration

[DanielThomas]

AOT retains registrations of BeanDefinitionRegistryPostProcessor beans, causing them to run again under AOT. Other than meaning you don't realise the benefits of avoidance of post-processing, if those processors register definitions unconditionally they fail with:

***************************
APPLICATION FAILED TO START
***************************

Description:

The bean 'GrpcClient_myclient' could not be registered. A bean with that name has already been defined and overriding is disabled.

Action:

Consider renaming one of the beans or enabling overriding by setting spring.main.allow-bean-definition-overriding=true

Appears that Boot is handling this conditionally in several places with AotDetector, but that appears to be explicitly for internal use.

In this specific case, it feels like AOT should filter the registrations, but what's the recommendation for similar patterns in end-user implementations?

• spring-framework/spring-core/src/main/java/org/springframework/aot/AotDetector.java

  Lines 22 to 29 in c1014f5

  	/**
  	* Utility for determining if AOT-processed optimizations must be used rather
  	* than the regular runtime. Strictly for internal use within the framework.
  	*
  	* @author Stephane Nicoll
  	* @since 6.0
  	*/
  	public abstract class AotDetector {

• https://github.com/spring-projects/spring-boot/blob/2c0edfd0b5046ecf4ecaaba628e63aea323922ac/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/SharedMetadataReaderFactoryContextInitializer.java#L65-L67
• https://github.com/search?q=repo%3Aspring-projects%2Fspring-boot+AotDetector.useGeneratedArtifacts&type=code

Example project:

https://github.com/DanielThomas/spring-aot-issues/tree/dannyt/post-processor-duplicate

Run and note the failure:

./gradlew bootJar && java -Dspring.aot.enabled=true -jar build/libs/demo-0.0.1-SNAPSHOT.jar

[sdeleuze]

Looks like related to #29484 and something we should potentially improve in Spring Framework 6.1. For your use case, in order to avoid the use of AotDetector internal API, could you maybe evaluate if you can update your BeanDefinitionRegistryPostProcessor to skip the processing if already applied (register a bean definition only if it does not exists, change existing beans only if not already done, etc.)?

[DanielThomas]

Most of our actual implementations do that already. We're also trying to avoid the overhead of running the processors at all:

We also have ContextCustomizer implementations that will have the same problem for tests, and looks like that's where Boot uses the AotDetector heavily. Is there a plan to provide a convention for user implementations to indicate they only do things that AOT is capturing?

[sdeleuze]

I think what will come up from #29484 should be usable for user implementation as well, maybe you could comment on it to bring this need to the attention of the team and we close this one as duplicate?

[snicoll]

@DanielThomas thanks for the report. We haven't really made up our mind on BeanDefinitionRegistryPostProcessor. I agree a "regular" use of the interface should be filtered out. Unfortunately, this isn't always the case, with implementations mutating the bean definitions in a way that AOT can't process them. Ideally, the "static" bean registration use case should be separated from the "runtime" bean mutation use case. It isn't easy as there aren't clear boundaries in the core API for this either.

I'd like this one to be kept open so that we can do an overview of our own implementations.

[snicoll]

I did a bit of searching in the codebase to at least what Spring Boot integrates with directly.

Post processors disabled with AotDetector:

• org.springframework.boot.test.autoconfigure.actuate.observability.ObservabilityContextCustomizerFactory.NoopTracerRegistrar
• org.springframework.boot.test.autoconfigure.jdbc.TestDatabaseAutoConfiguration.EmbeddedDataSourceBeanFactoryPostProcessor
• org.springframework.boot.test.graphql.tester.HttpGraphQlTesterContextCustomizer.HttpGraphQlTesterRegistrar
• org.springframework.boot.test.web.client.TestRestTemplateContextCustomizer.TestRestTemplateRegistrar
• org.springframework.boot.test.web.reactive.server.WebTestClientContextCustomizer.WebTestClientRegistrar

Post processors excluded explicitly via BeanRegistrationExcludeFilter:

• org.springframework.boot.autoconfigure.SharedMetadataReaderFactoryContextInitializer.CachingMetadataReaderFactoryPostProcessor
• org.springframework.integration.config.DefaultConfiguringBeanFactoryPostProcessor
• org.springframework.integration.config.IntegrationConfigurationBeanFactoryPostProcessor

Post processors excluded explicitly in the core infrastructure:

org.springframework.context.annotation.ConfigurationClassPostProcessor.

Other cases

org.springframework.boot.autoconfigure.webservices.WebServicesAutoConfiguration.WsdlDefinitionBeanFactoryPostProcessor registers bean definitions based on a property and use an instance supplier so it will fail if used with AOT.

org.springframework.boot.test.context.ImportsContextCustomizer.ImportsCleanupPostProcessor doesn't seem to be excluded but since it removes bean definitions once configuration class processing has completed, it should definitely be.

org.springframework.boot.context.ConfigurationWarningsApplicationContextInitializer.ConfigurationWarningsPostProcessor looks a legit use case of running them twice. Altough, an AOT contribution that stores the warning rather than running the check twice would be more appropriate.

org.springframework.integration.config.MessagingAnnotationPostProcessor is using AotDetector but not to prevent it to run with AOT. Rather it uses it to change the behavior of the post-processor.

org.springframework.security.config.debug.SecurityDebugBeanFactoryPostProcessor is logging a warning and is mutating a bean definition. While the former is required, the latter is not as it has been captured by AOT.

org.springframework.security.config.http.HttpSecurityBeanDefinitionParser.RequestRejectedHandlerPostProcessor is likely to be wrong at this stage as it runs twice and it probably shouldn't.

org.springframework.security.config.http.OAuth2ClientWebMvcSecurityPostProcessor is also to be reviewed. Unless the attributes that it sets on a bean definition can't be handled by AOT, it shouldn't run twice.

org.springframework.security.config.method.GlobalMethodSecurityBeanDefinitionParser.LazyInitBeanDefinitionRegistryPostProcessor is running twice and shouldn't. It's mutating the bean definition and that's captured by AOT.

org.springframework.security.config.websocket.WebSocketMessageBrokerSecurityBeanDefinitionParser.MessageSecurityPostProcessor should be reviewed. I believe it registers bean definitions programmatically and shouldn't run again with AOT.

org.springframework.security.oauth2.server.authorization.config.annotation.web.configuration.RegisterMissingBeanPostProcessor should be reviewed as well for the same reason.

Mixing contracts

Looking at whether they implement a new contract, this is clean with a few exceptions:

• org.springframework.integration.config.DefaultConfiguringBeanFactoryPostProcessor shouldn't be a smart initializing singleton and whatever that's doing should be put on a separate class
• org.springframework.integration.config.MessagingAnnotationPostProcessor is both a registry post processor and a bean post processor. The latter should definitely be around when AOT runs so splitting them would help.

Conclusion

There seem to be some possible improvements in our own codebase but it's not really straightforward to think about AOT when designing such a contract. Perhaps excluding such implementations automatically could help as it'll "force" users to decouple the behaviour, or avoid runtime mutation of the bean registry. I don't know if we can actually make such a change in a non major release and how we'd let folks opt-in for this to run again.

[snicoll]

Related #30372