ClassCastException possible when converting to a target type that is a subtype of T for a Converter<S, T> convertible type. [SPR-8718]

[spring-projects-issues]

Keith Donald opened SPR-8718 and commented

test case:

• register a StringToColorConverter implementing Converter<String, Color>
• ask the conversion service to convert a String to a MyColor, where MyColor extends Color.
• a class cast exception will occur, because a Color instance will be returned, which is not a MyColor.

instead of a class cast exception, a converter not found exception should be thrown.

---

Affects: 3.0.6

Issue Links:

• GenericConversionService should match target converters for compatible subclasses [SPR-8675] #13317 GenericConversionService should match target converters for compatible subclasses

[spring-projects-issues]

Keith Donald commented

public static class FileToInputStreamSourceConverter implements Converter<File, InputStreamSource> {
public InputStreamSource convert(File source) {
return new InputStreamSource() {
public InputStream getInputStream() throws IOException {
return null;
}
};
}
}

@Test
// @Ignore
public void testNoClassCastException() {
// #13317
conversionService.addConverter(new FileToInputStreamSourceConverter());
File file = new File("readme.txt");
Resource res = conversionService.convert(file, Resource.class); // class cast exception!
}

Rossen Stoyanchev 11:13 AM
can the ConversionService tell if a converter is capable of handling subtypes ?
i.e. if it accepts the target type

Keith Donald 11:13 AM
well you coudl argule this is user error above
i mean the guy is saying i want a converter to handle file-> to all input stream sources
but not implementing a converter factroy or generic converter
that's just a damn user error
but i'm wondering is there a way we can catch this ahead of time and let user know
not sure

Rossen Stoyanchev 11:15 AM
If I simply read the Converter javadoc though i wouldn't know the difference .. maybe this is a good use case to mention there .. i.e. if you want to support general to specific type conversion consider a ConverterFactory i think most people would read the Converter first

Keith Donald 11:15 AM
we cover that in the ref manual i ndetail

Rossen Stoyanchev 11:16 AM
ok

Keith Donald 11:16 AM
i mean i guess in ConverterAdapter we could check and make sure the returned value from the converter is assignable to the targetType
i guess that is the best we can do here...

Rossen Stoyanchev 11:17 AM
where would the check be?

Keith Donald 11:17 AM
ConverterAdapter

Rossen Stoyanchev 11:17 AM
in the convert method?

Keith Donald 11:17 AM
yes

Rossen Stoyanchev 11:18 AM
this adapter is specifically for simple Converters, right? it seems appropriate to make such a check

Keith Donald 11:18 AM
yes
i mean lets take another example
lets take StringToLocale instead...
so lets say I come up with SmartLocale, which extends Locale
now I try to bind a String to a SmartLocale
well StringToLocale is a regualr Converter
indexed under <String, Locale>
its gonna match, since SmartLocale is a Locale
yet its going to return a Locale, which is not assignable to a SmartLocale
so you'll get the ClassCastException there as well.

Rossen Stoyanchev 11:20 AM
maybe simple converters shouldn't be registered at all for target subtypes

Keith Donald 11:20 AM
the way around this is to register either a ConverterFactory aware of SmartLocale
or a specific StringToSmartLocale converter.
well what's a subtype?
any type that is not final can be extended, we can't control that
there is a converter registered for String->Locale and it works fine.
It allows you to bind a String to a Locale.
The issue comes in when you introduce a SmartLocale subclass.
String->Locale is still going to match, based on the algorithm.
The algorithm is uniform across all converter implementations, as each specific converter SPI is adapted to a GenericConverter.
it just so happens that a Converter can't know you want a SmartLocale, because it does not get passed that context...

Rossen Stoyanchev 11:23 AM
the "algorithm" is in the ConversionService matching logic?

Keith Donald 11:23 AM
yeah, GenericConversionService
all the lgoic to match a converter

Rossen Stoyanchev 11:23 AM
right
we go up the hierarchy
first we look for String->SmartLocale...
we don't find shit
so now we go up
we look for String->Locale...
ah, there it is.
we could say though "this is a regular converter, not a converter factory or generic converter", so skip.
but we don't now.

Rossen Stoyanchev 11:27 AM
short of the ConversionService being able to ask the converter if it can convert a specific type .. i don't see any other option but to raise a meaningful error
what we could do is have ConverterAdapter implement ConditionalGenericConverter and onlhy match if the desired targetType equals the convertible pair targetType.
lets see if that would work
so if we have String->SmartLocale binding.

Rossen Stoyanchev 11:28 AM
yeah, something like that sounds good ..

Keith Donald 11:28 AM
and a String->Locale converter registered.
we'd try String->SmartLocale first...no match
String->Locale second.. we'd find the converter, it'd be conditional, and we'd ask ... can you convert a String to a SmartLocale... and it would say no, since it only knows about regular Locales.

Rossen Stoyanchev 11:29 AM
I think that's preferable to a ClassCastException

Keith Donald 11:30 AM
well if that works, that's great as it allows us to leave the uniform conversion matching algorithm unchanged
i'd rather not going in and screw with that algorithm.
unless we have to

[spring-projects-issues]

Rossen Stoyanchev commented

ConverterAdapter is now a ConditionalGenericConverter that matches assignable source types and equal target types.

[spring-projects-issues]

Sam Brannen commented

Rossen,

Your changes to GenericConversionService broke NumberFormattingTests.testDefaultNumberFormatting().

Please investigate the reason and:

• either remove @Ignore from the test method declaration,
• or modify the test so that it passes.

Update: FormattingConversionServiceTests.testPrintNull() is also broken now and @Ignore'd as well.

[spring-projects-issues]

Sam Brannen commented

Set priority to critical since changes made in conjunction with this issue broke regression tests.

[spring-projects-issues]

Keith Donald commented

This issue isn't directly related to the 3.0.6 regression in the area of type conversion.

[spring-projects-issues]

Sam Brannen commented

Note that registering a formatter for Integer instead of Number as follows allows the test to pass:

conversionService.addFormatterForFieldType(Integer.class, new NumberFormatter());

But Number.class should work as well, since it did up until the recent change.

[spring-projects-issues]

Rossen Stoyanchev commented

Conditional converters are intended to act as an override for default "by type" converters (e.g. @NumberFormat overrides default conversion logic). A regular Converter should not be adapted as a conditional converter since that gives it a higher priority than other default "by type" converters (e.g. Printer, Parser).

[spring-projects-issues]

Rossen Stoyanchev commented

Since a simple Converter is not capable of converting to a target sub-type, GenericConversionService now ensures the target type equals simple Converter target types.

[spring-projects-issues]

Juergen Hoeller commented

Backported to 3.0.7, for release in late December.

Juergen