Multiple Converters from same source type to different Collection types not properly supported [SPR-11369]

[spring-projects-issues]

Brandon Zeeb opened SPR-11369 and commented

The GenericConversionService has a private static class ConverterCacheKey which is used to map source and target TypeDescriptors to a GenericConverter. This class is used to aggregate the hashCodes of two TypeDescriptors (source and target).

The current TypeDescriptor hashCode only looks at the bare Java Class<?>, which is fine for primitives and none collection types. In the case of Collections types with generics, only using the bare Java type of the Collection makes it impossible to register two Converters with the same target collection type.
ie:

1. Converter<String, Set<String>>
2. Converter<String, Set<Integer>>

The field resolvableType contains information on both the Collection type and it's generic types. Perhaps this field should be used instead?

I have rated this priority initially as "Critical" given it is not possible to register Converters with the same Collection type that differ on generic types, which is a blocker in our applications. We need this to work, and would prefer not to only accept Collection<String>.

---

No further details from SPR-11369

[spring-projects-issues]

Juergen Hoeller commented

I haven't looked at the details here yet, but generally speaking, a hashCode() implementation does not have to take all applicable characteristics into account - just an equals(...) implementation does, still to be called if hashCode() happened to return the same result.

There may still be a problem with registering such 'overloaded' Converters. However, I suspect it's not caused by that hashCode() implementation.

Juergen

[spring-projects-issues]

Brandon Zeeb commented

Hi Juergen. I agree generally that Objects need not represent all their fields within hashCode() and equals(Object o) methods. Please allow me to speak to this problem in more detail and explain why the TypeDescriptor#hashCode is at the center of this problem.

Simple example:

class ConverterDemo {

    static void main(def args) {
        GenericConversionService gcs = new GenericConversionService()
        gcs.addConverter(new StringToSetOfStringConverter())
        gcs.addConverter(new StringToSetOIntConverter())

        def x = gcs.convert("1,2,3,4", Set)
        println x
        println x.class
        println x.first().class

        def y = gcs.convert("a,b,c", TypeDescriptor.collection(Set, TypeDescriptor.valueOf(String)))
        println y
        println y.class
        println y.first().class
    }
}

class StringToSetOfStringConverter implements Converter<String, Set<String>> {
    @Override
    Set<String> convert(String source) {
        StringUtils.commaDelimitedListToSet(source)
    }
}

class StringToSetOIntConverter implements Converter<String, Set<Integer>> {
    @Override
    Set<Integer> convert(String source) {
        StringUtils.commaDelimitedListToSet(source).collect {
            it as Integer
        }
    }
}

One would expect the output to be:

1,2,3,4
java.util.HashSet
java.util.Integer

a,b,c
java.util.HashSet
java.util.String

Instead, we get a NumberFormatException when converting "a,b,c" since the Set of Integer Converter is used. Note, it is the last item to be added to the ConversionService.

Starting in GenericConverter, when we add a Converter, the following code is called:

@Override
public void addConverter(Class<?> sourceType, Class<?> targetType, Converter<?, ?> converter) {
	GenericConverter.ConvertiblePair typeInfo = new GenericConverter.ConvertiblePair(sourceType, targetType);
	addConverter(new ConverterAdapter(typeInfo, converter));
}

This creates a new ConvertiblePair (as mentioned above) and places it within the private Map<ConverterCacheKey, GenericConvergeter> converterCache field. As ConverterCacheKey is used as a hash key, it's hashCode method is very important:

ConvertiblePair#hashCode(), which is correct.

@Override
public int hashCode() {
    return this.sourceType.hashCode() * 31 + this.targetType.hashCode();
}

In the snippet above this.targetType is a TypeDescriptor. To the best of my knowledge, when Spring Webmvc binds to a Command Object, this will also be true, since we've replicated the problem in a production app as well as the contrived demo above.

So let's look at how TypeDescriptor build's it's hashCode (as used in ConvertiblePair).

TypeDescriptor#hashCode()

@Override
public int hashCode() {
	return getType().hashCode();
}

In the code above, getType() will only return the Class<?> for the container type, but not it's generics. This causes the converter for <String, Set<Integer>> to be placed in the same Map slot as the Converter<String, Set<String>>, ie: a map collision occurs.

Please let me know if you need any additional information or if I can help in any other way.

Thanks, Brandon.

[spring-projects-issues]

Juergen Hoeller commented

I can confirm that it's not possible to register converters from the same source type to multiple collection target types. However, this is arguably not a bug but rather an enhancement along the lines of Spring 4's generic type matching for dependency injection. Either way, we'll try to make this work for 4.0.2; if it turns out to be a rather complex change, we'll have to defer to 4.1 instead.

Juergen

[spring-projects-issues]

Brandon Zeeb commented

That sounds great, thanks Juergen.

[spring-projects-issues]

Juergen Hoeller commented

This turned out as a rather straightforward change in ConverterAdapter, using a full ResolvableType assignability check in its matches(TypeDescriptor sourceType, TypeDescriptor targetType) implementation now. The specific converters have all been registered before even in case of target type overlaps; they just haven't been evaluated properly.

To be available in the next 4.0.2 snapshot. Please give it a try... (See http://projects.spring.io/spring-framework/ for Maven coordinates)

Juergen