Repository navigation
Conversation
Made MessageHandlerMethodFactory injectable into MessagingMethodInvokerHelper Deprecated HandlerMethodArgumentResolversHolder Added 'integrationMessageHandlerMethodFactory' property to IntegrationContextUtils
garyrussell
left a comment
garyrussell
left a comment
There was a problem hiding this comment.
Just one issue.
@artembilan You or I can fix this on merge if you find nothing else.
| if (isProvidedMessageHandlerFactoryBean()) { | ||
| this.messageHandlerMethodFactory = beanFactory.getBean(IntegrationContextUtils.MESSAGE_HANDLER_FACTORY_BEAN_NAME, | ||
| MessageHandlerMethodFactory.class); | ||
| } |
There was a problem hiding this comment.
I don't think we can do this here; the beanFactory might not have the bean yet.
Move to initialize?
There was a problem hiding this comment.
I thought about it and was struggling as well, but i guess you're right.
In all fairness I think this class needs a bit more surgery. Look at line 262 where messageHandlerMethodFactory is used which means we will createInvocableHandlerMethod always using the default one, yet may still inject one for the actual invocation. But I stop short of trying to make a big change. Perhaps "Consider revisiting..." type issue?
There was a problem hiding this comment.
Definitely better to move this getBean() into the initialize() below. This is not a "one-stop-shop" bean and it really may have a lot of dependencies, so calling it from the setBeanFactory() is a potential bug.
There was a problem hiding this comment.
I'll move it but keep in mind that it is still broken given what I said above. Ideally the logic that uses messageHandlerMethodFactory needs to be revisited/refactored so in the end it is a binary choice (default or injected). Right now it is kind of "not-really" a binary choice and it will be even more "not-really" if pushed to the initialize() method, since by that time you will execute at least two operations on the messageHandlerMethodFactory which in the end may not be used at all.
|
In future, please use imperative present tense: "Make MessageHandlerMethodFactory injectable" See the contribution guidelines and its link to Pro Git: https://git-scm.com/book/en/v2/Distributed-Git-Contributing-to-a-Project |
artembilan
left a comment
artembilan
left a comment
There was a problem hiding this comment.
Some feedback from me.
I can't insist, but I may merge this only after fixing setBeanFactory() concern and good argument about integrationMessageHandlerMethodFactory bean definition.
Thanks
|
|
||
| public static final String DISPOSABLES_BEAN_NAME = "integrationDisposableAutoCreatedBeans"; | ||
|
|
||
| public static final String MESSAGE_HANDLER_FACTORY_BEAN_NAME = "integrationMessageHandlerMethodFactory"; |
There was a problem hiding this comment.
Wait. And where is the bean for this name?
What is the point then to deprecate that HandlerMethodArgumentResolversHolder if we don't provide a valuable alternative out-of-the-box?
There was a problem hiding this comment.
Well, we do by allowing injection of integrationMessageHandlerMethodFactory (see HandlerMethodArgumentResolversHolder deprecation comment)
There was a problem hiding this comment.
What do you mean by "good argument about integrationMessageHandlerMethodFactory bean definition."?
There was a problem hiding this comment.
My point is that you just don't register the bean for the integrationMessageHandlerMethodFactory as out-of-the-box, default solution.
This way it doesn't sound reasonable to deprecate another feature since we really don't replace it.
| if (isProvidedMessageHandlerFactoryBean()) { | ||
| this.messageHandlerMethodFactory = beanFactory.getBean(IntegrationContextUtils.MESSAGE_HANDLER_FACTORY_BEAN_NAME, | ||
| MessageHandlerMethodFactory.class); | ||
| } |
There was a problem hiding this comment.
Definitely better to move this getBean() into the initialize() below. This is not a "one-stop-shop" bean and it really may have a lot of dependencies, so calling it from the setBeanFactory() is a potential bug.
| DefaultMessageHandlerMethodFactory handlerMethodFactory = new DefaultMessageHandlerMethodFactory(); | ||
| context.registerBean(IntegrationContextUtils.MESSAGE_HANDLER_FACTORY_BEAN_NAME, | ||
| MessageHandlerMethodFactory.class, () -> handlerMethodFactory); | ||
| context.refresh(); |
There was a problem hiding this comment.
The context has to be closed in the end of the test.
|
Actually I am withdrawing this PR. The problem I described above (the "always" usage of the default instance of As far as the good news that whole setBeanFactory() issue is not really an issue since it forces the creation of the bean as long as its definition is in the bean factory which will always be there (XML or annotation) if defined. Also, I believe it's late enough in the cycle to not worry about forcing the creation of a bean. But in any event this may change with new PR anyway. |
That's fully not true. This callback is called in the application context lifecycle pretty early and if the bean you try to get from there might have (and looks like it is) many other bean dependencies, we are going to end up with the "early bean factory access syndrome" anyway. |
|
Superseded with #2688 |
3 participants
Made MessageHandlerMethodFactory injectable into MessagingMethodInvokerHelper
Deprecated HandlerMethodArgumentResolversHolder
Added 'integrationMessageHandlerMethodFactory' property to IntegrationContextUtils