outbound-channel-adapter should support the 'expression' attribute [INT-1906]

[spring-operator]

Mark Fisher opened INT-1906 and commented

It should be mutually exclusive with 'ref+method' OR an inner bean.

---

Issue Links:

• add ability for MethodInvokingOutboundChannelAdapterParser to parse value of expression attribute for outbound-channel-adapter [INT-1971] #5934 add ability for MethodInvokingOutboundChannelAdapterParser to parse value of expression attribute for outbound-channel-adapter
  ("duplicates")
• add support for 'expression' on outbound-channel-adapter [INT-1907] #5893 add support for 'expression' on outbound-channel-adapter
  ("is duplicated by")
• Manual par. 3.2: add description for Channel Adapters abount ability SpEL evaluations [INT-2273] #6263 Manual par. 3.2: add description for Channel Adapters abount ability SpEL evaluations

[spring-operator]

Mark Fisher commented

Moving to 2.1. This will require something like ExpressionEvaluatingOutboundChannelAdapter.

[spring-operator]

Oleg Zhurakousky commented

Moving it to 2.1.M2.

Note to self:
The proper way to address it would be to:

1. Bring MethodInvokingOutboundChannelAdapterParser into AbstractDelegatingConsumerEndpointParser hierarchy (e.g., such as ServiceActivatorParser etc.)
2. Construct FactoryBean in the parser (just like we do with other MessageHandlers)
3. Tweak AbstractDelegatingConsumerEndpointParser or newly created FactoryBean to ensure it enforces adapters constraints (e.g., can not have a non-void method)

If we do all that than this issue will fix itself since expression handling is already a part of AbstractDelegatingConsumerEndpointParser

[spring-operator]

Oleg Zhurakousky commented

outbound-channel-adapter must enforce 'void' return type which is simple to do with ref+method and not possible with expression.
If one still needs to benefit from expression while maintaining the same behavior as outbound-channel-adapter use service-activator with requires-reply=false

[spring-operator]

Artem Bilan commented

outbound-channel-adapter is a good end-point of end the message flow and it should support SpEL as well as POJO-method invokation.
Sample is in duplicated issue https://jira.springsource.org/browse/INT-1971
Potential ExpressionEvaluatingOutboundChannelAdapter must expect expression result as it's described in Oleg's comment

[spring-operator]

Artem Bilan commented

Can I also refactor and fix MethodInvokingInboundChannelAdapterParser in this issue?
And also: how about to rename these both channelAdapters to 'Default' instead of 'MethodInvoking'?

[spring-operator]

Artem Bilan commented

I've tried to implement some ExpressionEvaluatingMessageHandler with delegation to ExpressionEvaluatingMessageProcessor with Void.class as expectedType and I've found that on backgound it is invoking BeanFactoryTypeConverter.convertValue.
The first two lines of that method are about checking targetType on Void, but there is no any check of sourceType in this case.
it may issue?

[spring-operator]

Artem Bilan commented

And I've found that MethodInvokingMessageHandler also doesn't check method return type...

[spring-operator]

Mark Fisher commented

When constructed with a Method instance it does check the return type:

public MethodInvokingMessageHandler(Object object, Method method) {
    Assert.isTrue(method.getReturnType().equals(void.class),
        "MethodInvokingMessageHandler requires a void-returning method");
...

However, it is typically constructed with a method NAME instead of a Method object, and that means it only checks each time it invokes the method (via its MethodInovkingMessageProcessor. In those cases, it does check whether the method returned a value at runtime:

protected void handleMessageInternal(Message<?> message) throws Exception {
    Object result = processor.processMessage(message);
    if (result != null) {
        throw new MessagingException(message, "the MethodInvokingMessageHandler method must "
                + "have a void return, but '" + this + "' received a value: [" + result + "]");			
    }
}

[spring-operator]

Artem Bilan commented

Hello, Mark
But it is abnormal when my mathod can return null, and we assume that this is void.
I was confused when I saw passed test:
https://github.com/SpringSource/spring-integration/blob/master/spring-integration-core/src/test/java/org/springframework/integration/config/ChannelAdapterParserTests.java on methodInvokingConsumer()
On the background org.springframework.integration.config.TestBean.store() +isn't void+.

[spring-operator]

Mark Fisher commented

What we might want to consider is whether we should delegate to SpEL for the method invocation. Then we could make sure the expected return type (void.class) is passed along to the SpEL evaluation so that it's not just allowing a null return, but actually enforcing a void return type (still at runtime).

[spring-operator]

Artem Bilan commented

So, Mark
Can you make resolution, what should I do within this issue?
As you can see I've found some confused points:

1. Rename or not MethodInvokingChannelAdapterParsers ?
2. Implement expression-support for MethodInvokingOutboundChannelAdapterParser ?
3. MethodInvokingInboundChannelAdapterParser doesn't have some mutually exclusive options
4. Implement enforcing a void return type ?

Or what do you think about this?

[spring-operator]

Mark Fisher commented

I would say the following:

1. rename MethodInvokingOutboundChannelAdapterParser to something like DefaultOutboundChannelAdapterParser (now that it will be method-invoking OR expression-evaluating).
2. Add expression support to the renamed parser
3. if 'expression' is provided, then 'ref'+'method' cannot be provided (and vice versa)
4. enforce the void return for the ExpressionEvaluating case by ensuring the expectedType is Void.class (that can be passed to the constructor of ExpressionEvaluatingMessageProcessor, which is what you would use under the covers for the 'expression' case - maybe within a new ExpressionEvaluatingMessageHandler)

[spring-operator]

Artem Bilan commented

Ok, Mark
Thanks for your time
But I've already done all the tasks you described.
Now I have only one problem: BeanFactoryTypeConverter.convertValue doesn't check sourceType with targetType and always return null.
Should I fix it also?
If not: to set ExpressionEvaluatingMessageProcessor.expectedType = Void.class will be useless.

Maybe should I do some PR about my work and we continue the discussion on source code lines ;-) ?

[spring-operator]

Oleg Zhurakousky commented

Artem

I am actually moving it back to RC1 since this is actually a bug which should be fixed either by enforcing Void return type as you've experimented or by removing 'expression' attribute from the namespace. So see hwta you can do and submit a PR and we'll take it from there.