INT-2433: Ridding off MessageProcessors wrapping #720

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants
*/
-abstract class AbstractMessageProcessingSelector implements MessageSelector, BeanFactoryAware {
+public class MessageProcessingSelector implements MessageSelector, BeanFactoryAware {

This comment has been minimized.

Show comment Hide comment
@garyrussell

garyrussell Feb 8, 2013

Member

It's not clear to me why are renaming these base classes - just because they have no abstract methods doesn't mean it makes sense to allow instantiation of the base class.

@garyrussell

garyrussell Feb 8, 2013

Member

It's not clear to me why are renaming these base classes - just because they have no abstract methods doesn't mean it makes sense to allow instantiation of the base class.

This comment has been minimized.

Show comment Hide comment
@artembilan

artembilan Feb 9, 2013

Member

They are explicit strategies to use any provided processor implementation. In our case it's about scripting processors. From other side Expression and MethodInvoking are some special cases.
Before there was no any ability to provide a processor implementation to these components: Splitter, Transfromer etc. It wrapped our processor to MethodInvokingProcessor through MethodInvokingHelper in the framework. And we should use the same technique in our applications, e.g. for MessageTransformingChannelInterceptor.
And that was why I pushed createMessageProcessingHandler() to subclasses of the AbstractStandardMessageHandlerFactoryBean.
I was thinking to introduce new implementations for these abstract classes, but they were looking very simple and redundant as their logic fully covered in the base classes.
I understand that it's more easier to review new code than refactoring of an existed one, especially if it looks working and legitimate.
But as you may have noticed, this refactoring doesn't breake any tests and also it is some internal change, which won't break exist applications with almost absolute guarantee.

@artembilan

artembilan Feb 9, 2013

Member

They are explicit strategies to use any provided processor implementation. In our case it's about scripting processors. From other side Expression and MethodInvoking are some special cases.
Before there was no any ability to provide a processor implementation to these components: Splitter, Transfromer etc. It wrapped our processor to MethodInvokingProcessor through MethodInvokingHelper in the framework. And we should use the same technique in our applications, e.g. for MessageTransformingChannelInterceptor.
And that was why I pushed createMessageProcessingHandler() to subclasses of the AbstractStandardMessageHandlerFactoryBean.
I was thinking to introduce new implementations for these abstract classes, but they were looking very simple and redundant as their logic fully covered in the base classes.
I understand that it's more easier to review new code than refactoring of an existed one, especially if it looks working and legitimate.
But as you may have noticed, this refactoring doesn't breake any tests and also it is some internal change, which won't break exist applications with almost absolute guarantee.

- ((AbstractMessageProcessor<Boolean>) this.messageProcessor).setConversionService(conversionService);
- }
- }
-

This comment has been minimized.

Show comment Hide comment
@garyrussell

garyrussell Feb 8, 2013

Member

Why can we no longer explicitly set the conversionService?

@garyrussell

garyrussell Feb 8, 2013

Member

Why can we no longer explicitly set the conversionService?

This comment has been minimized.

Show comment Hide comment
@artembilan

artembilan Feb 9, 2013

Member

This method was never invoked and its logic moved to setBeanFactory as it is in the other 'processing' strategies.
Comes to mind an idea to introduce IntegrationConversionServiceAware, implement it in the 'AbstractExpressionEvaluator', eliminate similar code everywhere, and in the potential setIntegrationConversionService make a check if the conversionService wasn't setted before manually.

@artembilan

artembilan Feb 9, 2013

Member

This method was never invoked and its logic moved to setBeanFactory as it is in the other 'processing' strategies.
Comes to mind an idea to introduce IntegrationConversionServiceAware, implement it in the 'AbstractExpressionEvaluator', eliminate similar code everywhere, and in the potential setIntegrationConversionService make a check if the conversionService wasn't setted before manually.

This comment has been minimized.

Show comment Hide comment
@artembilan

artembilan Feb 12, 2013

Member

BTW, about conversionService, I was thinking about https://jira.springsource.org/browse/INT-1639 and there was an idea to introduce some IntegrationEvaluationContextAware, where BeanFactory and ConversionService have been comming from injection of some IntegrationEvaluationContext prototype bean with <spel-function> ability. In most cases we need conversionService exactly for SpEL. So, it will be another big refactoring throughout the framework...

@artembilan

artembilan Feb 12, 2013

Member

BTW, about conversionService, I was thinking about https://jira.springsource.org/browse/INT-1639 and there was an idea to introduce some IntegrationEvaluationContextAware, where BeanFactory and ConversionService have been comming from injection of some IntegrationEvaluationContext prototype bean with <spel-function> ability. In most cases we need conversionService exactly for SpEL. So, it will be another big refactoring throughout the framework...

@garyrussell

This comment has been minimized.

Show comment Hide comment
@garyrussell

garyrussell Feb 8, 2013

Member

General comment: For a change like this, I think we need a more comprehensive commit comment - for example, a note saying why the createMessageProcessingHandler() method was pushed down to the subclasses would have been useful. Also a note about the Abstract class name changes.

Member

garyrussell commented Feb 8, 2013

General comment: For a change like this, I think we need a more comprehensive commit comment - for example, a note saying why the createMessageProcessingHandler() method was pushed down to the subclasses would have been useful. Also a note about the Abstract class name changes.

@artembilan

This comment has been minimized.

Show comment Hide comment
@artembilan

artembilan Feb 9, 2013

Member

Regarding this change, please, see the JIRA and it's related links.
Agree, I'll make commit comment more comprehensive.

Member

artembilan commented Feb 9, 2013

Regarding this change, please, see the JIRA and it's related links.
Agree, I'll make commit comment more comprehensive.

@garyrussell

This comment has been minimized.

Show comment Hide comment
@garyrussell

garyrussell Feb 14, 2013

Member

Artem, I'm sorry to say we have to push this to M2 - I need @markfisher to give it the 👍 because he was deeply involved in the original discussion last year. But, he has been super-busy and hasn't had time. We really need to get M1 out today, so this will miss the cut; sorry.

Member

garyrussell commented Feb 14, 2013

Artem, I'm sorry to say we have to push this to M2 - I need @markfisher to give it the 👍 because he was deeply involved in the original discussion last year. But, he has been super-busy and hasn't had time. We really need to get M1 out today, so this will miss the cut; sorry.

@artembilan

This comment has been minimized.

Show comment Hide comment
@artembilan

artembilan Feb 14, 2013

Member

Gary, np.
If I'd strongly need some solution I'd said about it.
BTW the same about my HTTP PR.
So, if you say I'll push these JIRAs to M2.

Have a good release!

Member

artembilan commented Feb 14, 2013

Gary, np.
If I'd strongly need some solution I'd said about it.
BTW the same about my HTTP PR.
So, if you say I'll push these JIRAs to M2.

Have a good release!

@garyrussell

This comment has been minimized.

Show comment Hide comment
@garyrussell

garyrussell Feb 14, 2013

Member

Thanks for your understanding :)

Member

garyrussell commented Feb 14, 2013

Thanks for your understanding :)

@garyrussell

This comment has been minimized.

Show comment Hide comment
@garyrussell

garyrussell Apr 25, 2013

Member

Hi Artem, we're finally getting back around to looking at these again. Sorry it took so long.

Mark took a look yesterday and we both agree it is very hard to review and it's really not clear why the changes need to be this extensive. (For example, I still don't understand why the abstract classes were made concrete - even with your explanation).

I am sure it's hard for you too to go back and look at this.

The real question is do we really need to make such a big change when we can avoid the wrapping with a simple instanceof check?

Member

garyrussell commented Apr 25, 2013

Hi Artem, we're finally getting back around to looking at these again. Sorry it took so long.

Mark took a look yesterday and we both agree it is very hard to review and it's really not clear why the changes need to be this extensive. (For example, I still don't understand why the abstract classes were made concrete - even with your explanation).

I am sure it's hard for you too to go back and look at this.

The real question is do we really need to make such a big change when we can avoid the wrapping with a simple instanceof check?

@artembilan

This comment has been minimized.

Show comment Hide comment
@artembilan

artembilan Apr 26, 2013

Member

Hi, all!

when we can avoid the wrapping with a simple instanceof check?

Gary, sounds like a bolt from the blue. :-)
I'll take a look today or on the weekend.
I mean if you're goint to release today, so let's push this to the future!

Member

artembilan commented Apr 26, 2013

Hi, all!

when we can avoid the wrapping with a simple instanceof check?

Gary, sounds like a bolt from the blue. :-)
I'll take a look today or on the weekend.
I mean if you're goint to release today, so let's push this to the future!

@artembilan

This comment has been minimized.

Show comment Hide comment
@artembilan

artembilan Apr 26, 2013

Member

Let me show an ugly sample from my app on the matter, to think :-):

<channel id="hsmGatewayChannel">
        <interceptors>
            <beans:bean class="org.springframework.integration.transformer.MessageTransformingChannelInterceptor">
                <beans:constructor-arg>
                    <beans:bean class="org.springframework.integration.transformer.MethodInvokingTransformer">
                        <beans:constructor-arg>
                            <beans:bean
                                    class="org.springframework.integration.groovy.GroovyScriptExecutingMessageProcessor">
                                <beans:constructor-arg>
                                    <beans:bean class="org.springframework.scripting.support.StaticScriptSource">
                                        <beans:constructor-arg>
                                            <beans:value>
                                                <![CDATA[
                                                    [account: payload['cardNum'],
                                                     expiration: payload['expDate'],
                                                     code: payload['serviceCode']?.toString(),
                                                     PIN: payload['pin']?.toString(),
                                                     PVV: payload['pvv']?.toString(),
                                                     PVKI: payload['pvki']?.toString()].findAll { it.value }
                                                    ]]>
                                            </beans:value>
                                        </beans:constructor-arg>
                                        <beans:constructor-arg value="HsmTransformingChannelInterceptorGroovyClass"/>
                                    </beans:bean>
                                </beans:constructor-arg>
                            </beans:bean>
                        </beans:constructor-arg>
                        <beans:constructor-arg value="processMessage"/>
                    </beans:bean>
                </beans:constructor-arg>
            </beans:bean>
        </interceptors>
    </channel>

From a start I forgot to say to the MethodInvokingTransformer additional constructor argument methodName and my application at runtime determines it as setCustomizer.
BTW it looks confused to use MethodInvoking... strategy around MessageProcessor implementation.
As I understood you correctly, your suggestion is to change MethodInvokingMessageProcessor to use MessageProcessor directly. Or in the MessagingMethodInvokerHelper. Right?

Member

artembilan commented Apr 26, 2013

Let me show an ugly sample from my app on the matter, to think :-):

<channel id="hsmGatewayChannel">
        <interceptors>
            <beans:bean class="org.springframework.integration.transformer.MessageTransformingChannelInterceptor">
                <beans:constructor-arg>
                    <beans:bean class="org.springframework.integration.transformer.MethodInvokingTransformer">
                        <beans:constructor-arg>
                            <beans:bean
                                    class="org.springframework.integration.groovy.GroovyScriptExecutingMessageProcessor">
                                <beans:constructor-arg>
                                    <beans:bean class="org.springframework.scripting.support.StaticScriptSource">
                                        <beans:constructor-arg>
                                            <beans:value>
                                                <![CDATA[
                                                    [account: payload['cardNum'],
                                                     expiration: payload['expDate'],
                                                     code: payload['serviceCode']?.toString(),
                                                     PIN: payload['pin']?.toString(),
                                                     PVV: payload['pvv']?.toString(),
                                                     PVKI: payload['pvki']?.toString()].findAll { it.value }
                                                    ]]>
                                            </beans:value>
                                        </beans:constructor-arg>
                                        <beans:constructor-arg value="HsmTransformingChannelInterceptorGroovyClass"/>
                                    </beans:bean>
                                </beans:constructor-arg>
                            </beans:bean>
                        </beans:constructor-arg>
                        <beans:constructor-arg value="processMessage"/>
                    </beans:bean>
                </beans:constructor-arg>
            </beans:bean>
        </interceptors>
    </channel>

From a start I forgot to say to the MethodInvokingTransformer additional constructor argument methodName and my application at runtime determines it as setCustomizer.
BTW it looks confused to use MethodInvoking... strategy around MessageProcessor implementation.
As I understood you correctly, your suggestion is to change MethodInvokingMessageProcessor to use MessageProcessor directly. Or in the MessagingMethodInvokerHelper. Right?

@artembilan

This comment has been minimized.

Show comment Hide comment
@artembilan

artembilan Sep 1, 2013

Member

Closed
The issue will be revised after merge #857

Member

artembilan commented Sep 1, 2013

Closed
The issue will be revised after merge #857

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment