Fix BeanFactory propagation for MMInvokerHelper #2694
Conversation
| @@ -56,7 +56,10 @@ public void setBeanFactory(BeanFactory beanFactory) throws BeansException { | |||
| public T processMessage(Message<?> message) { | |||
| if (this.delegate == null) { | |||
| BeanFactory beanFactory = getBeanFactory(); | ||
| return beanFactory != null | ||
| && beanFactory.containsBean(IntegrationContextUtils.MESSAGE_HANDLER_FACTORY_BEAN_NAME); | ||
| return getBeanFactory().containsBean(IntegrationContextUtils.MESSAGE_HANDLER_FACTORY_BEAN_NAME); | ||
| } |
There was a problem hiding this comment.
Given the extensive changes to the tests, this looks like it's a big breaking change. Should we retain the null check until 5.2?
There was a problem hiding this comment.
Even if we have such a check here, that doesn't mean that we are good to live with the propagation bug in the BeanNameMessageProcessor and RecipientListRouter. I mean this with the beanFactory != null we jsut mask the real problem in several places and we may have some questions in the future from community.
Not sure why massive changes in the tests scares you to merge it as a bug into the current version...
More over some tests have critical issues with not closed application context which may cause sporadic failures on CI.
There was a problem hiding this comment.
My point is that we'll get an NPE, which is never good.
Perhaps add an Assert.state() in initialize() and also remove the null checks there?
There was a problem hiding this comment.
Right. That is my next step after some review here. We really don't have consistency over there.
The point is that without BF propagation our method invocations might be different and that could be some kind of surprise for end-users.
But I really can relax the requirement here and bring check back. As long as I have a fix for propagation and we are protected in tests already 😄
So, do we have a deal to merge this into the current version if I revert NPe check in the MessagingMethodInvokerHelper?
We really can make it strict then in the next version catching more non-propagated bean factory if any...
There was a problem hiding this comment.
So, do we have a deal to merge this into the current version if I revert NPe check in the MessagingMethodInvokerHelper?
We really can make it strict then in the next version catching more non-propagated bean factory if any...
Agreed.
* Remove check for `null` in the `MessagingMethodInvokerHelper.isProvidedMessageHandlerFactoryBean()` * Fix `RecipientListRouter` for `BeanFactory` propagation to the `Recipient.selector` * Fix tests for `BeanFactory` population and propagation * Add `errorChannel` into the `TestUtils.createTestApplicationContext()` * Fix some Sonar smell, including new reported
`MessagingMethodInvokerHelper` to avoid breaking changes in the current point release * Some other polishing and optimizations in the `MessagingMethodInvokerHelper`
artembilan
force-pushed
the
BeanFactoryPropagation
branch
from
2011ee9 to
ab94a68
Compare
|
Pushed the fix as we discussed after rebasing. |
nullin theMessagingMethodInvokerHelper.isProvidedMessageHandlerFactoryBean()RecipientListRouterforBeanFactorypropagation to theRecipient.selectorBeanFactorypopulation and propagationerrorChannelinto theTestUtils.createTestApplicationContext()