ConversionService.canConvert(..) is inconsistent with the behavior of ConversionService.convert(..) method. [SPR-7548]

[spring-projects-issues]

Oleg Zhurakousky opened SPR-7548 and commented

This might actually be a bug, but for now i am treating it as an improvement request.

Basically there are couple of scenarios when canConvert(..) method could return 'false' while the actual conversion is still possible. For example conversion of an empty parameterized or un-parameterized collection to a collection parameterized with different type or even unparameterized collection.

A more realistic scenario is when using SpEL to invoke method with parameterized collection as argument while passing differently parameterized or un-parameterized but EMPTY collection. See code below

private static class MethodWithCollection{
    public void methodWithCollection(List<Foo> fooCollection){}
}
// test code
List<Bar> emptyArray = new ArrayList<Bar>();
MethodParameter methodParam = new MethodParameter(MethodWithCollection.class.getDeclaredMethod("methodWithCollection", List.class), 0);
GenericConversionService conversionService = ConversionServiceFactory.createDefaultConversionService();
boolean canConvert = conversionService.canConvert(emptyArray, new TypeDescriptor(methodParam));.

The above will actually return FALSE However, if you still decide to continue with conversion it will happen successfully:

Object convertedA = conversionService.convert(emptyArray, TypeDescriptor.forObject(emptyArray), new TypeDescriptor(methodParam));

It will simply return an empty unconverted collection and rightfully so since empty collection is unconvertible and no attempt to convert it should be made.. In fact I believe this is closely related to the recent change in #12146.

So as you can see there is inconsistency between canConvert(..) and convert(..) methods

Unfortunately today there is no way for the _ConversionService.canConvert(..) method to know wether we are dealing with en empty collection or not since there is no method that takes the source object as an argument, only source type.

So I would propose to add a new method to a ConversionService

boolean canConvert(Object source, TypeDescriptor targetType);

as well as few extra modifications to accommodate the above scenario.

Attached is a patch which takes care of this issue with additional test cases. It passes full Spring build.

---

Affects: 3.0.4

Attachments:

• SPR-7548.txt (9.89 kB)

Issue Links:

• INT-1441 NPE in BeanFactoryTypeConverter when upgrading to Spring 3.0.5 ("is depended on by")

• 12146 StackOverflowError, ClassCastException and other issues with ConversionService

Referenced from: commits https://github.com/spring-projects/spring-integration/commit/4263202e717e42b1d69ef5d4778b68c83d251ad7

1 votes, 5 watchers

[spring-projects-issues]

Chris Beams commented

Possibly related to recent fixes in #12146

[spring-projects-issues]

Chris Beams commented

Juergen - sounds like this may be a genuine bug, possibly spurred on by the recent changes in #12146. Looks like Oleg's patch takes care of it, but best to have you review directly if we're going to drop this in 3.0.5.

[spring-projects-issues]

Juergen Hoeller commented

Hmm, in principle, why not use canConvert(TypeDescriptor.forObject(emptyArray), new TypeDescriptor(methodParam))? canConvert doesn't really need access to the actual source object; it just needs access to the type information derived from the source object in this specific case... And with respect to SpEL, as of 3.0.5, we should be exposing the full type information there now, i.e. TypeDescriptor.forObject(...) which would make this information available already.

So I guess we're just missing a check for an empty array somewhere in TypeDescriptor (which it may derive from the source object that it wraps), exposing it in a way such that GenericConversionService is able to consider the information in its canConvert implementation? That would avoid the need for yet another canConvert variant which I would prefer for conceptual as well as for practical reasons.

Juergen

[spring-projects-issues]

Oleg Zhurakousky commented

Juergen, I tried that approach initially (modification to TypeDescriptor). The issue is that in this unique case we are not trying to determine if type A is convertible to type B. We actually don't care if it is. We are simply stating that empty Collection\ could be represented as Collection<?> without any conversion simply based on the fact that it is empty and its type information is not available. And even if it was available there is really nothing to convert. So we are simply returning TRUE always strictly based on the fact that the source Collection is empty. However if it was not empty we would attempt a real conversion.

Also, I think it would make the API more consistent. If there is a way to convert(Object to Type), then i should be able to inquire if i canConvert(Object to Type). Would you agree?

Juergen Hoeller commented

Let's revisit this for 3.1, since I really don't want to introduce any new methods on an SPI interface in 3.0.x.

In general, canConvert is meant to be usable even when just comparing type signatures (e.g. on startup, for validation purposes or for caching structural information), whereas convert is what you'll use when you actually got a value.

I guess the question is: What are you typically going to do if canConvert returns false for a specific value, right before attempting the convert call? Is there a fallback then, avoiding the use of the ConversionService altogether? Otherwise you could simply call convert and deal with an exception thrown from it...

Juergen

[spring-projects-issues]

Oleg Zhurakousky commented

Great point and this is essentially where I was heading. Here is a bit of a different spin where in a way I am questioning what is the value of canConvert() method in the first place? Because it seems like regardless if it returns TRUE or FALSE, you can't know for sure unless the actual conversion is attempted. In other words in the above scenario it returns FALSE, but conversion method succeeds. And on the flip side i can register StringToInteger converter resulting in the successful canConvert() invocation while still failing to convert non-numeric string value. So, I think the true solution would be not in adding another method to an API, but actually renaming the existing method to something that better describes the purpose of this method, something along the lines of supportsConversion(source, target). Because in our case we did call canConvert() because there is a fallback strategy which resulted in NPE and we have to fix it on SI side anyway. But in the particular scenario that triggered this issue if we were to skip canConvert() and call convert(), we would get successful result and no NPE. So, the name of the method now does imply that it is a convenience method that one might call before the actual call to convert() to avoid dealing with Exception. But in reality this method has a completely different meaning and that is to say that "this conversion service supports (not guarantees) conversion from type A to type B" and therefore its current name is somewhat misleading.

[spring-projects-issues]

Juergen Hoeller commented

Since there's nothing concrete to do here, I'll close the issue. Point taken that canConvert's name isn't ideal but we're not going to rename it any time soon either.

Juergen