spring-projects
A BeanPostProcessor should not create bean definitions
Right now, MessagingBeanPostProcessor is creating bean definitions. In order to do that, it has to inject the BeanFactory and downcast it to a BeanRegistry, see https://github.com/spring-projects/spring-integration/blob/7f631fc947c1b4c5f284b887aa144ba4c100486c/spring-integration-core/src/main/java/org/springframework/integration/config/annotation/MessagingAnnotationPostProcessor.java#L233
As a side effect of this, we might be able to get rid of that @Bean lookup. @Bean is a configuration class parsing concern and shouldn't be relied upon outside of it.
I would generally recommend an approach that does not require an individual runtime bean registration per endpoint, e.g. a central endpoint registry bean that accepts every such endpoint (as e.g. in JmsListenerAnnotationBeanPostProcessor).
Most importantly, bean definitions are not meant to be registered with an existing instance: A specified bean supplier is meant to create a new instance for every call, or at least perform a fresh lookup for an existing bean instance every time. For the purposes here, it seems you could be using registerSingleton with such an existing endpoint instance. That would avoid the registry downcast since registerSingleton is designed to be available on ConfigurableBeanFactory, not requiring internal metadata management in the core container but rather expressing the intent of making a pre-existing bean instance available.
It always worked that way and I guess that's just because there is no way in Spring to register several beans at once.
However I don't think that with our current simple "annotation on method" approach end-user would like to do anything else.
The solution here really pre-dates what you show in the JmsListenerAnnotationBeanPostProcessor, however I don't think a central registry is what we'd like to pursue here.
We have many more stuff around (JMX, for example, integration graph), where we indeed need an independent bean for the endpoint based on a messaging annotation.
I probably can rework the logic to the registerSingleton(), but as far as I remember there were some problems, e.g. with removal endpoints at runtime.
All of these looks too noise just for one @Bean I exposed for native...
Just a side note: we also have a IntegrationFlowBeanPostProcessor which parses an IntegrationFlow bean and registers many other beans (or reuse existing) according ti its tree-like structure.
We also support over there a runtime flows registration (and removal), which definitely might not work in native images. So, not a question here.
What I'm trying to say that registration (and removal) of BeanDefiniiton at runtime was really great feature for us for all of these years so it sounds a bit frustrating that some limitations in AOT force us for an overhaul which may lead not where because there is no way to register several bean from one definition in Spring...
As a middle ground I can look into a MergedBeanDefinitionPostProcessor for the @Bean with messaging annotations use-case.
This way I won't parse a @Bean and just register endpoints according messaging annotations.
The mentioned MessagingAnnotationPostProcessor will be left just for POJO methods, but still free from a @Bean parsing.
Thank you for your patience!
Runtime registration and removal of bean definitions isn't going away but unfortunately it is just not ideal for AOT processing, by the very nature of the approach. From my perspective, we should use the opportunity to streamline our registration arrangements for better predictability at build time.
The post-processor interfaces always hinted at this by not exposing registry references where they are not meant to be used... but of course downcasts always worked, bypassing that guidance that we had in the SPI design. A BeanPostProcessor kicks in pretty late in the game, on creation of specific bean instances, at a point when the factory is already optimized for complete bean definition metadata. A BeanDefinitionRegistryPostProcessor would kick in much earlier, operating on the complete initial set of bean definitions and being able to derive further bean definitions, just like configuration class processing itself does.
Runtime features such as registerSingleton are in a different ballpark since they do not affect the metadata management, just instance availability for state management and dependency injection purposes. If late registrations need to happen, it's highly recommended to use either a shared registry bean that accepts those individual registrations or registerSingleton but not late bean definition metadata registrations. That's generally sensible and also naturally compatible with the AOT approach.
Thank you, @jhoeller , for kind explanation!
I will look into your suggestions for the next milestone.
In addition, I'll investigate if that would be possible to generate some code for those endpoint beans via BeanRegistrationAotProcessor when we have messaging annotations on them: https://docs.spring.io/spring-integration/docs/current/reference/html/configuration.html#annotations_on_beans.
This way we would have an optimized configuration for native image, plus there won't be need for that @Bean reflection.
Of course, it might be possible if BeanRegistrationAotContribution allows to generate the code for additional beans...
All of these looks too noise just for one @Bean I exposed for native...
For the record, this was just the trigger that made something totally unrelated to AOT apparent.
a bit frustrating that some limitations in AOT force us for an overhaul
I don't know which limitations you're talking about. None of this here has anything to do with AOT. The main purpose of this issue is the registration of bean definitions in a BeanPostProcessor. And if we manage to find another way to implement this feature, then the @Bean lookup will probably go away as a side-effect.
In addition, I'll investigate if that would be possible to generate some code
Please don't. For the first version, we really want the AOT engine to process contexts for various use cases in a generic fashion. Code should be written only for very specific use cases and we should do our best to improve the engine if that's not the case. Thanks!
No problem, @snicoll ! The code generation is still dark matter for me anyway :relaxed:
Then I will look into a BeanDefinitionRegistryPostProcessor for this @Bean with Messaging Annotations to register that mentioned extra endpoint bean definition.
Most importantly, bean definitions are not meant to be registered with an existing instance: A specified bean supplier is meant to create a new instance for every call...
For the purposes here, it seems you could be using
registerSingletonwith such an existing endpoint instance.
@jhoeller One benefit of using registerBean with a supplier returning a single instance is that the instance is initialized when retrieved. With registerSingleton, we have to cast the BF to an AutowireCapableBeanFactory (e.g. CLBF) to initialize it afterwards.
To help the transition back to registering singletons, perhaps a new API could be provided that also initializes the bean; e.g.
<T> T initializeAndAddBean(String name, T instance);
Even better if it was available on a top level interface (e.g. ApplicationContext) to avoid having to cast (even if the default implementation throws an UnsupportedOperationException, or the initialize part is a no-op there).
Another side of this medal is a destroySingleton(String beanName) which is not available in the ConfigurableBeanFactory - just on the DefaultSingletonBeanRegistry.
I don't mind any discussion at any place, but it looks like we are hijacking this issue for other mater, than a concern around bean definition registration in the BeanPostProcessor.
I'm looking these days into making the mentioned MessagingAnnotationPostProcessor as a BeanDefinitionRegistryPostProcessor as well to process @Bean with messaging annotation on early stage.
The work reminds me our old good XML parsers, but instead of Element I parse Annotation 😄 .
Well, doesn't look like the solution is fast enough, so no any promises for the milestone next week...