Backwards-compatible handling of generic and raw collection converters

Issue: SPR-11369

spring-core/src/main/java/org/springframework/core/convert/converter/ConditionalGenericConverter.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2012 the original author or authors.
+ * Copyright 2002-2014 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -19,9 +19,9 @@ package org.springframework.core.convert.converter;
 import org.springframework.core.convert.TypeDescriptor;
 
 /**
- * A {@link GenericConverter} that may conditionally execute based on attributes of the
- * {@code source} and {@code target} {@link TypeDescriptor}. See
- * {@link ConditionalConverter} for details.
+ * A {@link GenericConverter} that may conditionally execute based on attributes
+ * of the {@code source} and {@code target} {@link TypeDescriptor}.
+ * See {@link ConditionalConverter} for details.
  *
  * @author Keith Donald
  * @author Phillip Webb
@@ -29,7 +29,6 @@ import org.springframework.core.convert.TypeDescriptor;
  * @see GenericConverter
  * @see ConditionalConverter
  */
-public interface ConditionalGenericConverter extends GenericConverter,
-		ConditionalConverter {
+public interface ConditionalGenericConverter extends GenericConverter, ConditionalConverter {
 
 }

spring-core/src/main/java/org/springframework/core/convert/support/GenericConversionService.java

@@ -331,18 +331,18 @@ public class GenericConversionService implements ConfigurableConversionService {
 
 		@Override
 		public boolean matches(TypeDescriptor sourceType, TypeDescriptor targetType) {
-			ResolvableType rt = targetType.getResolvableType();
-			if (!rt.isAssignableFrom(this.targetType)) {
-				// Generic type structure not fully assignable -> try lenient fallback if
-				// unresolvable generics remain, just requiring the raw type to match then
-				if (!rt.hasUnresolvableGenerics() || !this.typeInfo.getTargetType().equals(targetType.getObjectType())) {
-					return false;
-				}
+			// Check raw type first...
+			if (!this.typeInfo.getTargetType().equals(targetType.getObjectType())) {
+				return false;
 			}
-			if (this.converter instanceof ConditionalConverter) {
-				return ((ConditionalConverter) this.converter).matches(sourceType, targetType);
+			// Full check for complex generic type match required?
+			ResolvableType rt = targetType.getResolvableType();
+			if (!(rt.getType() instanceof Class) && !rt.isAssignableFrom(this.targetType) &&
+					!this.targetType.hasUnresolvableGenerics()) {
+				return false;
 			}
-			return true;
+			return !(this.converter instanceof ConditionalConverter) ||
+					((ConditionalConverter) this.converter).matches(sourceType, targetType);
 		}
 
 		@Override

spring-core/src/test/java/org/springframework/core/convert/support/GenericConversionServiceTests.java

@@ -619,7 +619,7 @@ public class GenericConversionServiceTests {
 		GenericConversionService conversionService = new DefaultConversionService();
 		byte[] byteArray = new byte[] { 1, 2, 3 };
 		Byte[] converted = conversionService.convert(byteArray, Byte[].class);
-		assertTrue(Arrays.equals(converted, new Byte[] { 1, 2, 3 }));
+		assertTrue(Arrays.equals(converted, new Byte[] {1, 2, 3}));
 	}
 
 	@Test
@@ -739,7 +739,7 @@ public class GenericConversionServiceTests {
 		byte[] byteArray = new byte[] { 1, 2, 3 };
 		byte[] converted = conversionService.convert(byteArray, byte[].class);
 		assertNotSame(byteArray, converted);
-		assertTrue(Arrays.equals(new byte[] { 2, 3, 4 }, converted));
+		assertTrue(Arrays.equals(new byte[] {2, 3, 4}, converted));
 	}
 
 	@Test
@@ -769,8 +769,11 @@ public class GenericConversionServiceTests {
 
 	@Test
 	public void multipleCollectionTypesFromSameSourceType() throws Exception {
+		conversionService.addConverter(new MyStringToRawCollectionConverter());
+		conversionService.addConverter(new MyStringToGenericCollectionConverter());
 		conversionService.addConverter(new MyStringToStringCollectionConverter());
 		conversionService.addConverter(new MyStringToIntegerCollectionConverter());
+
 		assertEquals(Collections.singleton("testX"),
 				conversionService.convert("test", TypeDescriptor.valueOf(String.class), new TypeDescriptor(getClass().getField("stringCollection"))));
 		assertEquals(Collections.singleton(4),
@@ -788,6 +791,7 @@ public class GenericConversionServiceTests {
 	@Test
 	public void adaptedCollectionTypesFromSameSourceType() throws Exception {
 		conversionService.addConverter(new MyStringToStringCollectionConverter());
+
 		assertEquals(Collections.singleton("testX"),
 				conversionService.convert("test", TypeDescriptor.valueOf(String.class), new TypeDescriptor(getClass().getField("stringCollection"))));
 		assertEquals(Collections.singleton("testX"),
@@ -800,6 +804,46 @@ public class GenericConversionServiceTests {
 				conversionService.convert("test", TypeDescriptor.valueOf(String.class), new TypeDescriptor(getClass().getField("stringCollection"))));
 		assertEquals(Collections.singleton("testX"),
 				conversionService.convert("test", TypeDescriptor.valueOf(String.class), new TypeDescriptor(getClass().getField("rawCollection"))));
+
+		try {
+			conversionService.convert("test", TypeDescriptor.valueOf(String.class), new TypeDescriptor(getClass().getField("integerCollection")));
+			fail("Should have thrown ConverterNotFoundException");
+		}
+		catch (ConverterNotFoundException ex) {
+			// expected
+		}
+	}
+
+	@Test
+	public void genericCollectionAsSource() throws Exception {
+		conversionService.addConverter(new MyStringToGenericCollectionConverter());
+
+		assertEquals(Collections.singleton("testX"),
+				conversionService.convert("test", TypeDescriptor.valueOf(String.class), new TypeDescriptor(getClass().getField("stringCollection"))));
+		assertEquals(Collections.singleton("testX"),
+				conversionService.convert("test", TypeDescriptor.valueOf(String.class), new TypeDescriptor(getClass().getField("genericCollection"))));
+		assertEquals(Collections.singleton("testX"),
+				conversionService.convert("test", TypeDescriptor.valueOf(String.class), new TypeDescriptor(getClass().getField("rawCollection"))));
+
+		// The following is unpleasant but a consequence of the generic collection converter above...
+		assertEquals(Collections.singleton("testX"),
+				conversionService.convert("test", TypeDescriptor.valueOf(String.class), new TypeDescriptor(getClass().getField("integerCollection"))));
+	}
+
+	@Test
+	public void rawCollectionAsSource() throws Exception {
+		conversionService.addConverter(new MyStringToRawCollectionConverter());
+
+		assertEquals(Collections.singleton("testX"),
+				conversionService.convert("test", TypeDescriptor.valueOf(String.class), new TypeDescriptor(getClass().getField("stringCollection"))));
+		assertEquals(Collections.singleton("testX"),
+				conversionService.convert("test", TypeDescriptor.valueOf(String.class), new TypeDescriptor(getClass().getField("genericCollection"))));
+		assertEquals(Collections.singleton("testX"),
+				conversionService.convert("test", TypeDescriptor.valueOf(String.class), new TypeDescriptor(getClass().getField("rawCollection"))));
+
+		// The following is unpleasant but a consequence of the raw collection converter above...
+		assertEquals(Collections.singleton("testX"),
+				conversionService.convert("test", TypeDescriptor.valueOf(String.class), new TypeDescriptor(getClass().getField("integerCollection"))));
 	}
 
 
@@ -810,6 +854,7 @@ public class GenericConversionServiceTests {
 	public static @interface ExampleAnnotation {
 	}
 
+
 	private static class MyConditionalConverter implements Converter<String, Color>, ConditionalConverter {
 
 		private int matchAttempts = 0;
@@ -830,8 +875,8 @@ public class GenericConversionServiceTests {
 		}
 	}
 
-	private static class MyConditionalGenericConverter implements GenericConverter,
-			ConditionalConverter {
+
+	private static class MyConditionalGenericConverter implements GenericConverter, ConditionalConverter {
 
 		private List<TypeDescriptor> sourceTypes = new ArrayList<TypeDescriptor>();
 
@@ -857,8 +902,8 @@ public class GenericConversionServiceTests {
 		}
 	}
 
-	private static class MyConditionalConverterFactory implements
-			ConverterFactory<String, Color>, ConditionalConverter {
+
+	private static class MyConditionalConverterFactory implements ConverterFactory<String, Color>, ConditionalConverter {
 
 		private MyConditionalConverter converter = new MyConditionalConverter();
 
@@ -885,6 +930,7 @@ public class GenericConversionServiceTests {
 		}
 	}
 
+
 	interface MyEnumInterface {
 
 		String getCode();
@@ -900,6 +946,23 @@ public class GenericConversionServiceTests {
 		}
 	}
 
+
+	public static class MyStringToRawCollectionConverter implements Converter<String, Collection> {
+
+		@Override
+		public Collection convert(String source) {
+			return Collections.singleton(source + "X");
+		}
+	}
+
+	public static class MyStringToGenericCollectionConverter implements Converter<String, Collection<?>> {
+
+		@Override
+		public Collection<?> convert(String source) {
+			return Collections.singleton(source + "X");
+		}
+	}
+
 	private static class MyEnumInterfaceToStringConverter<T extends MyEnumInterface> implements Converter<T, String> {
 
 		@Override
@@ -924,12 +987,13 @@ public class GenericConversionServiceTests {
 		}
 	}
 
-	public Collection<String> stringCollection;
-
-	public Collection<Integer> integerCollection;
 
 	public Collection rawCollection;
 
 	public Collection<?> genericCollection;
 
+	public Collection<String> stringCollection;
+
+	public Collection<Integer> integerCollection;
+
 }