Allow use of @Transformer on void-returning methods [INT-635]

[spring-operator]

Dave Syer opened INT-635 and commented

Add @Enricher as a special @Transformer that just mutates its payload. E.g.

@Enricher
public void enrich(Trade trade) {
  trade.setISIN(getISINFromCode(trade.getCode()));
}

The signature is always void.

---

Affects: 1.0.2

Issue Links:

• Provide a MethodResolver implementation to the SpEL EvaluationContext that filters candidate Methods [INT-936] #4951 Provide a MethodResolver implementation to the SpEL EvaluationContext that filters candidate Methods
  ("depends on")

[spring-operator]

Mark Fisher commented

I wonder if we could just make this the regular behavior for a void-returning method annotated with @Transformer.

The only reason I hesitate to introduce @Enricher is that the technique of modifying just the payload itself is really more general than "enrichment" (even if that might be the most popular use-case). In fact, there may be cases where this is used to null-out certain fields to hide them from downstream handlers.

[spring-operator]

Dave Syer commented

Void @Transformer works for me. Good idea.

[spring-operator]

Iwein Fuld commented

I think as a general best practice payloads should be immutable. As long as we put that in the javadoc I'm fine with this feature though, it can be convenient I'm sure.

[spring-operator]

Mark Fisher commented

Interesting point Iwein. If this is implemented by extending AbstractTransformerPayload (as a method-invoking adapter), then it will indeed be creating a new Message for the transformed payload in the same exact way as using @Transformer on a method that does return a different payload instance. So, I think that should be clarified in the JavaDoc, but otherwise we should be ok. It's really just a convenience so that one can annotate a void method. That is a bit of a restrictive requirement for POJOs after all.

[spring-operator]

Mark Fisher commented

updated the issue's summary to match the use of @Transformer

[spring-operator]

Mark Fisher commented

This turns out to be a bit tricky with the current implementation approach. Basically the MethodInvokingTransformer is delegating to an invoker that ultimately calls invoke(..) on an instance of java.lang.reflect.Method. Of course, at that point, if the underlying method returns void, that invocation returns null. Since it also returns null, when the underlying method does return null, we cannot distinguish between those two outcomes at the level of MethodInvokingTransformer. So, it looks like time to reconsider either the way we delegate there or the way the underlying method invoker treats void return types (e.g. we could check the return type and always return some form of a result wrapper).

Bottom line: this is not as trivial as it seemed upon first glance ;)

[spring-operator]

Iwein Fuld commented

On second thought I think the difference between mutable and immutable payloads is too large to hide it like this. If someone wants to enrich he should

@Transformer
public Trade enrich(Trade trade) {
   //...
}

Nothing is stopping someone that really wants to from working with a mutable Trade, but at least we wouldn't encourage it. If we really want to support void returning methods here, we should be more clear than just javadoc, I think.

[spring-operator]

Mark Fisher commented

So, are you suggesting that a separate @Enrich annotation makes sense after all?

In either case, I think the cleanest way to handle the issue I raised above (void return via reflection as null) would be to use another adapter that invokes the void-returning method. I need to actually consider that within the code again, however, because the relationship to the dynamic checking of the type is a bit tricky (although in this case, the argument would have to be a payload and not an actual Message instance).

[spring-operator]

Mark Fisher commented

I'm deferring this to 2.0 since we should consider the options more carefully and also consider how this may relate to new features planned for 2.0 such as EL and annotations for return value binding.

[spring-operator]

Mark Fisher commented

I'm moving this to M3 since it will likely be much easier to implement once we have the concept of Method-filtering in the SpEL-driven MethodResolver (see #4951).

[spring-operator]

Oleg Zhurakousky commented

I think this is a typical case for Content Enrichment. Today we only address Headers portion of the content, but not the Payload.
Also, I think this is the only place where payload could be mutable since it is the only place where it could be enriched, everything else would be a different (new) payload.
Not only we would need annotation, but also xml support for it similar to header enricher.

[spring-operator]

Mark Fisher commented

Considering other Messages might be referencing the same instance, the potential repercussions seem too dangerous.