New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add protected methods to resolve the target type for payload arguments [SPR-17503] #22035
Comments
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 |
Artem Bilan commented First of all the 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 I will wait for your reply, but rough decision it to close as invalid. Thanks for understanding |
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
Anyway, feel free to close it and I'll raise it with Spring team |
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 What you would like to achieve is something like custom use-case, which has to be addressed with a separate Does it make sense? |
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 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
|
Artem Bilan commented Moved to SF for further possible discussion or more arguments to close it as invalid. Thanks |
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 |
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. |
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 Taking a closer look at that, the @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? |
Artem Bilan commented OK. So, any custom |
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. |
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. |
A problem with the suggestion I made previously is that with a @olegz looking at your changes, I'm not sure they make sense in the general case. What we want is to make sure is the payload can be assigned to the target type, i.e. it will not cause ClassCastException but otherwise can be more general. Not the other way around where if the payload type is more general than the target type, it would pass the check but fail with a ClassCastException. The way you were describing it makes more sense:
Specifically what I see here is a case where the target is What I can suggest is a method like this in
Another alternative would be to have special handling for target class of |
I have gone ahead and added protected methods for resolving the target class. |
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 actuallybyte[]
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
The text was updated successfully, but these errors were encountered: