Fix GenericConversionService search algorithm

Previously the algorithm used by GenericConversionService to find
converters incorrectly searched for interfaces working up from the
base class. This caused particular problems with custom List
converters as as the Collection interface would be considered before
the List interface giving CollectionToObjectConverter precedence
over the custom converter.

The updated algorithm restores the class search order to behave in the
same way as Spring 3.1.

Issue: SPR-10116

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

@@ -431,14 +431,6 @@ public class GenericConversionService implements ConfigurableConversionService {
 	 */
 	private static class Converters {
 
-		private static final Set<Class<?>> IGNORED_CLASSES;
-		static {
-			Set<Class<?>> ignored = new HashSet<Class<?>>();
-			ignored.add(Object.class);
-			ignored.add(Object[].class);
-			IGNORED_CLASSES = Collections.unmodifiableSet(ignored);
-		}
-
 		private final Set<GenericConverter> globalConverters =
 				new LinkedHashSet<GenericConverter>();
 
@@ -483,12 +475,13 @@ public class GenericConversionService implements ConfigurableConversionService {
 		 */
 		public GenericConverter find(TypeDescriptor sourceType, TypeDescriptor targetType) {
 			// Search the full type hierarchy
-			List<TypeDescriptor> sourceCandidates = getTypeHierarchy(sourceType);
+			List<Class<?>> sourceCandidates = getClassHierarchy(sourceType.getType());
-			List<TypeDescriptor> targetCandidates = getTypeHierarchy(targetType);
+			List<Class<?>> targetCandidates = getClassHierarchy(targetType.getType());
-			for (TypeDescriptor sourceCandidate : sourceCandidates) {
+			for (Class<?> sourceCandidate : sourceCandidates) {
-				for (TypeDescriptor targetCandidate : targetCandidates) {
+				for (Class<?> targetCandidate : targetCandidates) {
+					ConvertiblePair convertiblePair = new ConvertiblePair(sourceCandidate, targetCandidate);
 					GenericConverter converter = getRegisteredConverter(
-							sourceType, targetType, sourceCandidate, targetCandidate);
+							sourceType, targetType, convertiblePair);
 					if(converter != null) {
 						return converter;
 					}
@@ -497,12 +490,11 @@ public class GenericConversionService implements ConfigurableConversionService {
 			return null;
 		}
 
-		private GenericConverter getRegisteredConverter(TypeDescriptor sourceType, TypeDescriptor targetType,
+		private GenericConverter getRegisteredConverter(TypeDescriptor sourceType,
-				TypeDescriptor sourceCandidate, TypeDescriptor targetCandidate) {
+				TypeDescriptor targetType, ConvertiblePair convertiblePair) {
 
 			// Check specifically registered converters
-			ConvertersForPair convertersForPair = converters.get(new ConvertiblePair(
+			ConvertersForPair convertersForPair = converters.get(convertiblePair);
-				sourceCandidate.getType(), targetCandidate.getType()));
 			GenericConverter converter = convertersForPair == null ? null
 				: convertersForPair.getConverter(sourceType, targetType);
 			if (converter != null) {
@@ -512,7 +504,7 @@ public class GenericConversionService implements ConfigurableConversionService {
 			// Check ConditionalGenericConverter that match all types
 			for (GenericConverter globalConverter : this.globalConverters) {
 				if (((ConditionalConverter)globalConverter).matches(
-						sourceCandidate, targetCandidate)) {
+						sourceType, targetType)) {
 					return globalConverter;
 				}
 			}
@@ -526,45 +518,39 @@ public class GenericConversionService implements ConfigurableConversionService {
 		 * @return an ordered list of all classes that the given type extends or
 		 *         implements.
 		 */
-		private List<TypeDescriptor> getTypeHierarchy(TypeDescriptor type) {
+		private List<Class<?>> getClassHierarchy(Class<?> type) {
-			if(type.isPrimitive()) {
+			List<Class<?>> hierarchy = new ArrayList<Class<?>>(20);
-				type = TypeDescriptor.valueOf(type.getObjectType());
+			Set<Class<?>> visited = new HashSet<Class<?>>(20);
-			}
+			addToClassHierarchy(0, ClassUtils.resolvePrimitiveIfNecessary(type), false, hierarchy, visited);
-			Set<TypeDescriptor> typeHierarchy = new LinkedHashSet<TypeDescriptor>();
+			boolean array = type.isArray();
-			collectTypeHierarchy(typeHierarchy, type);
+			int i = 0;
-			if(type.isArray()) {
+			while (i < hierarchy.size()) {
-				typeHierarchy.add(TypeDescriptor.valueOf(Object[].class));
+				Class<?> candidate = hierarchy.get(i);
-			}
+				candidate = (array ? candidate.getComponentType()
-			typeHierarchy.add(TypeDescriptor.valueOf(Object.class));
+						: ClassUtils.resolvePrimitiveIfNecessary(candidate));
-			return new ArrayList<TypeDescriptor>(typeHierarchy);
+				Class<?> superclass = candidate.getSuperclass();
-		}
+				if (candidate.getSuperclass() != null && superclass != Object.class) {
-
+					addToClassHierarchy(i + 1, candidate.getSuperclass(), array, hierarchy, visited);
-		private void collectTypeHierarchy(Set<TypeDescriptor> typeHierarchy,
+				}
-				TypeDescriptor type) {
+				for (Class<?> implementedInterface : candidate.getInterfaces()) {
-			if(type != null && !IGNORED_CLASSES.contains(type.getType())) {
+					addToClassHierarchy(hierarchy.size(), implementedInterface, array, hierarchy, visited);
-				if(typeHierarchy.add(type)) {
+				}
-					Class<?> superclass = type.getType().getSuperclass();
+				i++;
-					if (type.isArray()) {
+			}
-						superclass = ClassUtils.resolvePrimitiveIfNecessary(superclass);
+			addToClassHierarchy(hierarchy.size(), Object.class, array, hierarchy, visited);
-					}
+			addToClassHierarchy(hierarchy.size(), Object.class, false, hierarchy, visited);
-					collectTypeHierarchy(typeHierarchy, createRelated(type, superclass));
+			return hierarchy;
-
+		}
-					for (Class<?> implementsInterface : type.getType().getInterfaces()) {
+
-						collectTypeHierarchy(typeHierarchy, createRelated(type, implementsInterface));
+		private void addToClassHierarchy(int index, Class<?> type, boolean asArray,
+				List<Class<?>> hierarchy, Set<Class<?>> visited) {
+			if(asArray) {
+				type = Array.newInstance(type, 0).getClass();
+			}
+			if(visited.add(type)) {
+				hierarchy.add(index, type);
 			}
 		}
-			}
-		}
-
-		private TypeDescriptor createRelated(TypeDescriptor type, Class<?> relatedType) {
-			if (relatedType == null && type.isArray()) {
-				relatedType = Array.newInstance(relatedType, 0).getClass();
-			}
-			if(!type.getType().equals(relatedType)) {
-				return type.upcast(relatedType);
-			}
-			return null;
-		}
 
 		@Override
 		public String toString() {

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

@@ -16,11 +16,7 @@
 
 package org.springframework.core.convert.support;
 
-import static junit.framework.Assert.assertEquals;
+import static org.junit.Assert.*;
-import static junit.framework.Assert.assertTrue;
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertNull;
-import static org.junit.Assert.assertSame;
 
 import java.awt.Color;
 import java.math.BigDecimal;
@@ -506,6 +502,21 @@ public class DefaultConversionTests {
 		assertEquals(source, result);
 	}
 
+	@Test
+	public void convertCollectionToObjectWithCustomConverter() throws Exception {
+		List<String> source = new ArrayList<String>();
+		source.add("A");
+		source.add("B");
+		conversionService.addConverter(new Converter<List, ListWrapper>() {
+			@Override
+			public ListWrapper convert(List source) {
+				return new ListWrapper(source);
+			}
+		});
+		ListWrapper result = conversionService.convert(source, ListWrapper.class);
+		assertSame(source, result.getList());
+	}
+
 	@Test
 	public void convertObjectToCollection() {
 		List<String> result = (List<String>) conversionService.convert(3L, List.class);
@@ -777,4 +788,17 @@ public class DefaultConversionTests {
 		}
 	}
 
+	private static class ListWrapper {
+
+		private List<?> list;
+
+		public ListWrapper(List<?> list) {
+			this.list = list;
+		}
+
+		public List<?> getList() {
+			return list;
+		}
+	}
+
 }

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

@@ -27,7 +27,6 @@ import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Iterator;
 import java.util.LinkedHashMap;
-import java.util.LinkedHashSet;
 import java.util.LinkedList;
 import java.util.List;
 import java.util.Map;
@@ -48,6 +47,7 @@ import org.springframework.core.io.Resource;
 import org.springframework.util.StopWatch;
 import org.springframework.util.StringUtils;
 
+import static org.hamcrest.Matchers.greaterThan;
 import static org.junit.Assert.*;
 
 /**
@@ -696,14 +696,11 @@ public class GenericConversionServiceTests {
 		MyConditionalGenericConverter converter = new MyConditionalGenericConverter();
 		conversionService.addConverter(converter);
 		assertEquals((Integer) 3, conversionService.convert(3, Integer.class));
+		assertThat(converter.getSourceTypes().size(), greaterThan(2));
 		Iterator<TypeDescriptor> iterator = converter.getSourceTypes().iterator();
-		assertEquals(Integer.class, iterator.next().getType());
-		assertEquals(Number.class, iterator.next().getType());
-		TypeDescriptor last = null;
 		while(iterator.hasNext()) {
-			last = iterator.next();
+			assertEquals(Integer.class, iterator.next().getType());
 		}
-		assertEquals(Object.class, last.getType());
 	}
 
 	@Test
@@ -784,7 +781,7 @@ public class GenericConversionServiceTests {
 	private static class MyConditionalGenericConverter implements GenericConverter,
 			ConditionalConverter {
 
-		private Set<TypeDescriptor> sourceTypes = new LinkedHashSet<TypeDescriptor>();
+		private List<TypeDescriptor> sourceTypes = new ArrayList<TypeDescriptor>();
 
 		public Set<ConvertiblePair> getConvertibleTypes() {
 			return null;
@@ -800,7 +797,7 @@ public class GenericConversionServiceTests {
 			return null;
 		}
 
-		public Set<TypeDescriptor> getSourceTypes() {
+		public List<TypeDescriptor> getSourceTypes() {
 			return sourceTypes;
 		}
 	}