Add protected methods to resolve the target type for payload arguments [SPR-17503]

[spring-projects-issues]

olegz opened SPR-17503 and commented

While working on https://github.com/spring-cloud/spring-cloud-stream/issues/1527 I noticed that MessageConversion was not attempted on handler with signature public Person echo(Object value) where the payload was actually byte[]

 

I believe it is due to the following code on line 130. I believe the order of arguments should be different if (ClassUtils.isAssignable(payloadClass, targetClass))

 

---

Affects: 5.1.2

[spring-projects-issues]

olegz commented

It's not a big problem for us and you may not consider it a bug after all, but I wanted you guys to take a look 

[spring-projects-issues]

Artem Bilan commented

First of all the PayloadArgumentResolver is a part of Spring Framework: https://github.com/spring-projects/spring-framework/blob/master/spring-messaging/src/main/java/org/springframework/messaging/handler/annotation/support/PayloadArgumentResolver.java

On the other hand the current logic fully reflects a Javadocs requirements:

/**
 * Check if the right-hand side type may be assigned to the left-hand side
 * type, assuming setting by reflection. Considers primitive wrapper
 * classes as assignable to the corresponding primitive types.
 * @param lhsType the target type
 * @param rhsType the value type that should be assigned to the target type
 * @return if the target type is assignable from the value type
 * @see TypeUtils#isAssignable
 */
public static boolean isAssignable(Class<?> lhsType, Class<?> rhsType) {

where the code is like this:

Class<?> targetClass = parameter.getParameterType();
Class<?> payloadClass = payload.getClass();
if (ClassUtils.isAssignable(targetClass, payloadClass)) {

and we really are going to convert an incoming payload to the parameter type.

On the other hand that is fully unclear from your description how byte[] is going to be converted to an Object? It is already one!

I will wait for your reply, but rough decision it to close as invalid.

Thanks for understanding

[spring-projects-issues]

olegz commented

Right, spring-framework, my bad. 

But just to close the loop, the issue is that we completely bypass MessageConverters in that case and that goes to the root of your question "how byte[] is going to be converted to an Object?". The answer is via MessageConverters or fail. The point is the in aforementioned SCSt issue and in similar issue in SCF we've come to realization that there is a special case where the actual conversion can happen without care for the actual input type. I am talking about content-type text/* where no matter what the input type is we should convert it to a textual representation, hence String, and that could be done in many different ways, but for consistency I was looking to do it via MessageConverter.

 

Anyway, feel free to close it and I'll raise it with Spring team

[spring-projects-issues]

Artem Bilan commented

That's not too hard to move this JIRA ticket over there into the SPR project.

Only the problem if there is the point to do that at all. The current logic is fully stable and reflects whatever we had so far. The byte[] is indeed an Object. That's how there is no any conversion at all.

What you would like to achieve is something like custom use-case, which has to be addressed with a separate ArgumentResolver, which is going to be registered before this default one PayloadArgumentResolver.

Does it make sense?

[spring-projects-issues]

olegz commented

Everything is an object. . . 

The argument I am making is that explicit definition of Object or ? as an input type could also mean "do not/can not use input type as a conversion driver, what is my content-type?" and if content-type is text/**' what else can it be other then String regardless of what the input type is? This essentially puts CT text/** in a special category, but then again some may disagree.  

So. no, I don't think (at the moment) that it is a custom case requiring custom argument resolver, but this also needs more discussion probably with spring team

 

[spring-projects-issues]

Artem Bilan commented

Moved to SF for further possible discussion or more arguments to close it as invalid.

Thanks

[spring-projects-issues]

Rossen Stoyanchev commented

So the idea is to always call the converter? So the converter can decide if it wants to perform some conversion even if the target type matches, as long as it ends up with something that still matches the target type.

I did a quick test with that change and didn't get any failing tests, but we may not have tests focusing on something like this. For example I know Jackson rejects conversion to Object since it doesn't know what kind of Object to create, but it might be smart enough to notice it can return the input. I didn't try.

We could also consider moving the payload vs target type check inside AbstractMessageConverter#fromMessage which would protect existing implementations, and allow others to override the behavior.

[spring-projects-issues]

olegz commented

Thank you Rosen

 

Yes, the idea is to always delegate to converter.

Having said that, I am not yet ready to pull any type of trigger on that. As trivial as it sounds there may be some traps. I just waned to make sure I'll document as much while it's fresh. 

let me think about what you and Artem both said and possibly come up with few tests and other tangible things that we can argue about. The fact that your quick smoke test didn't produce failures is encouraging though.

[spring-projects-issues]

Rossen Stoyanchev commented

I'm not sure the tests passing means all that much. I doubt we have tests targeting a plain Object.

My suggestion is simply to give converters the power to decide whether a conversion is necessary or not, which is what you're asking for essentially, and also goes along with the fact that the MessageConverter does not have a canConvert method in the first place. At the same time moving the isAssignable check from PayloadArgimentResolver to AbstractMessageConverter can protect existing converters just the same so it's a non-issue for existing code.

Taking a closer look at that, the fromMessage methods in AbstractMessageConverter are final so we can introduce a new protected method, and call it before calling convertFromInternal. Something like:

@Override
@Nullable
public final Object fromMessage(Message<?> message, Class<?> targetClass,
		@Nullable Object conversionHint) {

	if (!canConvertFrom(message, targetClass)) {
		return null;
	}
	return requiresConversion(message, targetClass) ?
			convertFromInternal(message, targetClass, conversionHint) : 
			message.getPayload();
}

protected boolean requiresConversion(Message<?> message, Class<?> targetClass) {
	Class<?> payloadClass = message.getPayload().getClass();
	return ClassUtils.isAssignable(targetClass, payloadClass);
}

Does this seem okay to everyone?

[spring-projects-issues]

Artem Bilan commented

OK. So, any custom MessageConverter can just ignore that logic and just do whatever it would like, for example Oleg's coercion to String when payload is byte[] and Content-Type is text/*.
Is that what would be outcome from this JIRA?

[spring-projects-issues]

Rossen Stoyanchev commented

Yes, it should be no impact to existing converters, but the choice to do whatever even if the target is already assignable from the payload.

[spring-projects-issues]

Oleg Zhurakousky commented

Rossen Stoyanchev, so we had a few issues in SCSt and I decided to jump the gun and provide those custom implementation of resolver(s) temporarily until this issue is resolved.

The good news is that it should provide you with additional validation as well as point of reference.

Here is the relevant commit - spring-cloud/spring-cloud-stream@a731021

Note that  aside form PayloadArgumentResolver, I needed to implement MessageMethodArgumentResolver as well. These two new resolvers may look busy (lot’s of code), but it's primarily since they are not easy to extend which may give you few ideas on API restructuring for cases where such extension may still be needed. I have few ideas but don't want to highjack the core issue with it.

The only relevant change is in the order of two parameters in:

ClassUtils.isAssignable(....) 

 

Anyway, we're good for now but i'll provide more feedback as it comes.

Once this issue is resolved we'll remove all that code.