INT-4571 Make MessageHandlerMethodFactory injectable #2688
Conversation
Make MessageHandlerMethodFactory injectable into MessagingMethodInvokerHelper Allow 'handlerMethod' to be overriden Deprecate HandlerMethodArgumentResolversHolder Add 'integrationMessageHandlerMethodFactory' property to IntegrationContextUtils Add test that actually validates that custom resolver gets picked up
| HandlerMethodArgumentResolversHolder handlerMethodArgumentResolversHolder = | ||
| beanFactory.getBean(this.canProcessMessageList | ||
| ? IntegrationContextUtils.LIST_ARGUMENT_RESOLVERS_BEAN_NAME | ||
| : IntegrationContextUtils.ARGUMENT_RESOLVERS_BEAN_NAME, |
There was a problem hiding this comment.
I'm still not OK do not have an out-of-the-box bean on the matter. And that would simplify a code in here, but at the same time pay attention, please, that we would like to use different argument resolvers for list case and non-list.
With the centralized single IntegrationContextUtils.MESSAGE_HANDLER_FACTORY_BEAN_NAME we don't have such a distribution and proper logic here in the in this class.
So, do we need to introduce an IntegrationContextUtils.LIST_MESSAGE_HANDLER_FACTORY_BEAN_NAME as well to cover this situation?
If I'm alone in my "brain swamp", feel free to merge it and let's raise an issue to revise it in the next version!
There was a problem hiding this comment.
I am not sure i follow. . .
We do not need OOTB bean. As I said in the notes it's status quo unless. . and the unless part is where you decide to override at which point you register a bean of type and name specified.
Yes, there are quite a lot of additional improvements that can/should be done in this specific class and I've mentioned some of them, but the impact of introducing it now (minor release) may be more then we can/should allow.
There was a problem hiding this comment.
The problem is that with an explicit IntegrationContextUtils.MESSAGE_HANDLER_FACTORY_BEAN_NAME bean in your target application you apply it for all the MessagingMethodInvokerHelper instances where some of them really should rely on the this.canProcessMessageList and the choice of argument resolves in your bean might be wrong or doesn't fit the list processing logic. I mean that with single global bean you are going to break something in your application without a way to reinstate the proper behavior.
I might agree if you only apply such a global bean for those MessagingMethodInvokerHelper which are not this.canProcessMessageList though...
There was a problem hiding this comment.
Correct, if I override MessageHandlerMethodFactory I am taking full control. I have to take care of everything - as actually shown in test where there is only single resolver which accepts everything.
There was a problem hiding this comment.
One more time: we have a special case for the this.canProcessMessageList.
See DefaultConfiguringBeanFactoryPostProcessor.internalArgumentResolversBuilder() and its logic around listCapable parameter.
I'm not sure that a single global bean can address that use-case. And that's definitely might be a case why we have fine-grained it into a separate IntegrationContextUtils.LIST_ARGUMENT_RESOLVERS_BEAN_NAME.
Might the case that I just can't find the proper words to explain the issue...
@garyrussell , WDYT?
There was a problem hiding this comment.
I agree that it would be better to pre-define standard beans for list and not-list case, and allow the user to override either one of them, or both; however, given the timing, and the fact we have to get 10 releases out today, I would tend to go with this compromise (SCSt doesn't need the list-capable invocation - that is generally for aggregation processors).
We can add an issue to the backlog to clean it up properly.
There was a problem hiding this comment.
OK. Then merging with simple code style polishing.
There was a problem hiding this comment.
I would also make sure we revisit the lifecycle of handleMethod so it's only created when the actual instance of MessageHandlerMethodFactory is known.
Now, with regard to standard beans for list and not-list case, I still believe it is out of scope for this issue and is the responsibility of the user who configures/overrides MessageHandlerMethodFactory to inject and configure the appropriate resolvers. IMHO it must be all or nothing. As an example, I as a user who provides my own instance of MessageHandlerMethodFactory would never expect framework doing anything to it (injecting additional resolvers, etc) other then just using it.
|
Merged as 0d13128 after some code style, smells and deprecations polishing. However with the |
Make MessageHandlerMethodFactory injectable into MessagingMethodInvokerHelper
Allow 'handlerMethod' to be overriden
Deprecate HandlerMethodArgumentResolversHolder
Add 'integrationMessageHandlerMethodFactory' property to IntegrationContextUtils
Add test that actually validates that custom resolver gets picked up
===
Quick NOTE: With the current implementation of MessagingMethodInvokerHelper by allowing injection of MessageHandlerMethodFactory we effectively providing a mechanism for overriding the
handlerMethodinstance that was already created in the constructor. While at the moment I don’t see a valid reason why it should be created in the constructor, an attempt to push it down toinitialize()method only resulted in some test failures which were asserting forhandlerMethodto NOT be null beforeinitialize()is called. In other words attempting to do that would be a much greater change, but I would consider revisiting the internal implementation and see if it is necessary to createhandlerMethodinside the constructor.In any event, this PR basically maintains the status quo aside from the changes described in the commit message and simply looks IF
MESSAGE_HANDLER_FACTORY_BEAN_NAMEexists during initialization and if it does it overrides the existingmessageHandlerMethodFactoryas well as the instance ofhandlerMethod.