Revised converter search algorithm to favor super classes before interface hierarchy

org.springframework.core/src/main/java/org/springframework/core/convert/TypeDescriptor.java

@@ -20,8 +20,10 @@ import java.lang.annotation.Annotation;
 import java.lang.reflect.Field;
 import java.util.Collection;
 import java.util.HashMap;
+import java.util.LinkedHashSet;
 import java.util.LinkedList;
 import java.util.Map;
+import java.util.Set;
 
 import org.springframework.core.GenericCollectionTypeResolver;
 import org.springframework.core.MethodParameter;
@@ -127,8 +129,8 @@ public class TypeDescriptor {
 	 * Use this factory method to introspect a source object's type before asking the conversion system to convert it to some another type.
 	 * Builds in population of nested type descriptors for collection and map objects through object introspection.
 	 * If the object is null, returns {@link TypeDescriptor#NULL}.
-	 * If the object is not a collection, simply calls {@link #valueOf(Class)}.
-	 * If the object is a collection, this factory method will derive the element type(s) by introspecting the collection.
+	 * If the object is not a collection or map, simply calls {@link #valueOf(Class)}.
+	 * If the object is a collection or map, this factory method will derive the element type(s) by introspecting the collection or map.
 	 * @param object the source object
 	 * @return the type descriptor
 	 * @see ConversionService#convert(Object, Class)
@@ -331,7 +333,7 @@ public class TypeDescriptor {
 		}
 		return this.mapValueType;
 	}
-
+	
 	// special case public operations
 
 	/**
@@ -536,6 +538,7 @@ public class TypeDescriptor {
 	}
 
 	private static Class<?> commonType(Class<?> commonType, Class<?> valueClass) {
+		Set<Class<?>> interfaces = new LinkedHashSet<Class<?>>();
 		LinkedList<Class<?>> classQueue = new LinkedList<Class<?>>();
 		classQueue.addFirst(commonType);
 		while (!classQueue.isEmpty()) {
@@ -543,21 +546,26 @@ public class TypeDescriptor {
 			if (currentClass.isAssignableFrom(valueClass)) {
 				return currentClass;
 			}
-			Class<?>[] interfaces = currentClass.getInterfaces();
-			for (Class<?> ifc : interfaces) {
-				addInterfaceHierarchy(ifc, classQueue);
-			}
-			if (currentClass.getSuperclass() != null) {
+			Class<?> superClass = currentClass.getSuperclass();
+			if (superClass != null && superClass != Object.class) {
 				classQueue.addFirst(currentClass.getSuperclass());
 			}
+			for (Class<?> interfaceType : currentClass.getInterfaces()) {
+				addInterfaceHierarchy(interfaceType, interfaces);
+			}
+		}
+		for (Class<?> interfaceType : interfaces) {
+			if (interfaceType.isAssignableFrom(valueClass)) {
+				return interfaceType;
+			}			
 		}
-		throw new IllegalStateException("Should never be invoked");	
+		return Object.class;
 	}
-	
-	private static void addInterfaceHierarchy(Class<?> ifc, LinkedList<Class<?>> classQueue) {
-		classQueue.addFirst(ifc);
-		for (Class<?> inheritedIfc : ifc.getInterfaces()) {
-			addInterfaceHierarchy(inheritedIfc, classQueue);
+
+	private static void addInterfaceHierarchy(Class<?> interfaceType, Set<Class<?>> interfaces) {
+		interfaces.add(interfaceType);
+		for (Class<?> inheritedInterface : interfaceType.getInterfaces()) {
+			addInterfaceHierarchy(inheritedInterface, interfaces);
 		}
 	}
 
@@ -577,13 +585,13 @@ public class TypeDescriptor {
 		this.fieldNestingLevel = nestingLevel;
 	}
 
-	public TypeDescriptor(Class<?> mapType, CommonElement commonKey, CommonElement commonValue) {
+	private TypeDescriptor(Class<?> mapType, CommonElement commonKey, CommonElement commonValue) {
 		this.type = mapType;
 		this.mapKeyType = applyCommonElement(commonKey);
 		this.mapValueType = applyCommonElement(commonValue);
 	}
 	
-	public TypeDescriptor(Class<?> collectionType, CommonElement commonElement) {
+	private TypeDescriptor(Class<?> collectionType, CommonElement commonElement) {
 		this.type = collectionType;
 		this.elementType = applyCommonElement(commonElement);
 	}
@@ -633,5 +641,5 @@ public class TypeDescriptor {
 		}
 		
 	}
-	
+
 }

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

@@ -20,7 +20,9 @@ import java.lang.reflect.Array;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.Iterator;
+import java.util.LinkedHashSet;
 import java.util.LinkedList;
 import java.util.List;
 import java.util.Map;
@@ -29,7 +31,6 @@ import java.util.concurrent.ConcurrentHashMap;
 
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
-
 import org.springframework.core.GenericTypeResolver;
 import org.springframework.core.convert.ConversionFailedException;
 import org.springframework.core.convert.ConversionService;
@@ -334,7 +335,7 @@ public class GenericConversionService implements ConversionService, ConverterReg
 			while (!classQueue.isEmpty()) {
 				Class<?> currentClass = classQueue.removeLast();
 				if (logger.isTraceEnabled()) {
-					logger.trace("Searching for converters indexed by sourceType [" + currentClass.getName() + "]");
+					logger.trace("Searching for converters indexed by source interface [" + currentClass.getName() + "]");
 				}
 				Map<Class<?>, MatchableConverters> converters = getTargetConvertersForSource(currentClass);
 				GenericConverter converter = getMatchingConverterForTarget(sourceType, targetType, converters);
@@ -349,40 +350,62 @@ public class GenericConversionService implements ConversionService, ConverterReg
 			Map<Class<?>, MatchableConverters> objectConverters = getTargetConvertersForSource(Object.class);
 			return getMatchingConverterForTarget(sourceType, targetType, objectConverters);
 		}
-		else {
+		else if (sourceObjectType.isArray()) {
 			LinkedList<Class<?>> classQueue = new LinkedList<Class<?>>();
 			classQueue.addFirst(sourceObjectType);
-			LinkedList<Class<?>> attemptedInterfaces = new LinkedList<Class<?>>();
 			while (!classQueue.isEmpty()) {
 				Class<?> currentClass = classQueue.removeLast();
 				if (logger.isTraceEnabled()) {
-					logger.trace("Searching for converters indexed by sourceType [" + currentClass.getName() + "]");
+					logger.trace("Searching for converters indexed by source array type [" + currentClass.getName() + "]");
 				}
 				Map<Class<?>, MatchableConverters> converters = getTargetConvertersForSource(currentClass);
 				GenericConverter converter = getMatchingConverterForTarget(sourceType, targetType, converters);
 				if (converter != null) {
 					return converter;
 				}
-				if (currentClass.isArray()) {
-					Class<?> componentType = ClassUtils.resolvePrimitiveIfNecessary(currentClass.getComponentType());
-					if (componentType.getSuperclass() != null) {
-						classQueue.addFirst(Array.newInstance(componentType.getSuperclass(), 0).getClass());
-					}
-					else if (componentType.isInterface()) {
-						classQueue.addFirst(Object[].class);
-					}
+				Class<?> componentType = ClassUtils.resolvePrimitiveIfNecessary(currentClass.getComponentType());
+				if (componentType.getSuperclass() != null) {
+					classQueue.addFirst(Array.newInstance(componentType.getSuperclass(), 0).getClass());
 				}
-				else {
-					Class<?>[] interfaces = currentClass.getInterfaces();
-					for (Class<?> ifc : interfaces) {
-						addInterfaceHierarchy(ifc, classQueue, attemptedInterfaces);
-					}
-					if (currentClass.getSuperclass() != null) {
-						classQueue.addFirst(currentClass.getSuperclass());
-					}
+				else if (componentType.isInterface()) {
+					classQueue.addFirst(Object[].class);
 				}
 			}
 			return null;
+		} else {
+			HashSet<Class<?>> interfaces = new LinkedHashSet<Class<?>>();
+			LinkedList<Class<?>> classQueue = new LinkedList<Class<?>>();
+			classQueue.addFirst(sourceObjectType);
+			while (!classQueue.isEmpty()) {
+				Class<?> currentClass = classQueue.removeLast();
+				if (logger.isTraceEnabled()) {
+					logger.trace("Searching for converters indexed by source class [" + currentClass.getName() + "]");
+				}
+				Map<Class<?>, MatchableConverters> converters = getTargetConvertersForSource(currentClass);
+				GenericConverter converter = getMatchingConverterForTarget(sourceType, targetType, converters);
+				if (converter != null) {
+					return converter;
+				}
+				Class<?> superClass = currentClass.getSuperclass();
+				if (superClass != null && superClass != Object.class) {
+					classQueue.addFirst(superClass);
+				}
+				for (Class<?> interfaceType : currentClass.getInterfaces()) {
+					addInterfaceHierarchy(interfaceType, interfaces);
+				}
+			}
+			for (Class<?> interfaceType : interfaces) {
+				if (logger.isTraceEnabled()) {
+					logger.trace("Searching for converters indexed by source interface [" + interfaceType.getName() + "]");
+				}
+				Map<Class<?>, MatchableConverters> converters = getTargetConvertersForSource(interfaceType);
+				GenericConverter converter = getMatchingConverterForTarget(sourceType, targetType, converters);
+				if (converter != null) {
+					return converter;
+				}
+			}
+			Map<Class<?>, MatchableConverters> objectConverters = getTargetConvertersForSource(Object.class);
+			return getMatchingConverterForTarget(sourceType, targetType, objectConverters);				
 		}
 	}
 
@@ -403,7 +426,7 @@ public class GenericConversionService implements ConversionService, ConverterReg
 			while (!classQueue.isEmpty()) {
 				Class<?> currentClass = classQueue.removeLast();
 				if (logger.isTraceEnabled()) {
-					logger.trace("and indexed by targetType [" + currentClass.getName() + "]");
+					logger.trace("and indexed by target class [" + currentClass.getName() + "]");
 				}
 				MatchableConverters matchable = converters.get(currentClass);
 				GenericConverter converter = matchConverter(matchable, sourceType, targetType);
@@ -419,51 +442,72 @@ public class GenericConversionService implements ConversionService, ConverterReg
 				logger.trace("and indexed by [java.lang.Object]");
 			}							
 			return matchConverter(converters.get(Object.class), sourceType, targetType);
+		} else if (targetObjectType.isArray()) {
+			LinkedList<Class<?>> classQueue = new LinkedList<Class<?>>();
+			classQueue.addFirst(targetObjectType);
+			while (!classQueue.isEmpty()) {
+				Class<?> currentClass = classQueue.removeLast();
+				if (logger.isTraceEnabled()) {
+					logger.trace("and indexed by target array type [" + currentClass.getName() + "]");
+				}				
+				MatchableConverters matchable = converters.get(currentClass);
+				GenericConverter converter = matchConverter(matchable, sourceType, targetType);
+				if (converter != null) {
+					return converter;
+				}
+				Class<?> componentType = ClassUtils.resolvePrimitiveIfNecessary(currentClass.getComponentType());
+				if (componentType.getSuperclass() != null) {
+					classQueue.addFirst(Array.newInstance(componentType.getSuperclass(), 0).getClass());
+				}
+				else if (componentType.isInterface()) {
+					classQueue.addFirst(Object[].class);
+				}
+			}
+			return null;
 		}
 		else {
+			Set<Class<?>> interfaces = new LinkedHashSet<Class<?>>();
 			LinkedList<Class<?>> classQueue = new LinkedList<Class<?>>();
 			classQueue.addFirst(targetObjectType);
-			LinkedList<Class<?>> attemptedInterfaces = new LinkedList<Class<?>>();			
 			while (!classQueue.isEmpty()) {
 				Class<?> currentClass = classQueue.removeLast();
 				if (logger.isTraceEnabled()) {
-					logger.trace("and indexed by targetType [" + currentClass.getName() + "]");
+					logger.trace("and indexed by target class [" + currentClass.getName() + "]");
 				}				
 				MatchableConverters matchable = converters.get(currentClass);
 				GenericConverter converter = matchConverter(matchable, sourceType, targetType);
 				if (converter != null) {
 					return converter;
 				}
-				if (currentClass.isArray()) {
-					Class<?> componentType = ClassUtils.resolvePrimitiveIfNecessary(currentClass.getComponentType());
-					if (componentType.getSuperclass() != null) {
-						classQueue.addFirst(Array.newInstance(componentType.getSuperclass(), 0).getClass());
-					}
-					else if (componentType.isInterface()) {
-						classQueue.addFirst(Object[].class);
-					}
+				Class<?> superClass = currentClass.getSuperclass();
+				if (superClass != null && superClass != Object.class) {
+					classQueue.addFirst(superClass);
 				}
-				else {
-					Class<?>[] interfaces = currentClass.getInterfaces();
-					for (Class<?> ifc : interfaces) {
-						addInterfaceHierarchy(ifc, classQueue, attemptedInterfaces);
-					}
-					if (currentClass.getSuperclass() != null) {
-						classQueue.addFirst(currentClass.getSuperclass());
-					}
+				for (Class<?> interfaceType : currentClass.getInterfaces()) {
+					addInterfaceHierarchy(interfaceType, interfaces);
 				}
 			}
-			return null;
+			for (Class<?> interfaceType : interfaces) {
+				if (logger.isTraceEnabled()) {
+					logger.trace("and indexed by target interface [" + interfaceType.getName() + "]");
+				}				
+				MatchableConverters matchable = converters.get(interfaceType);
+				GenericConverter converter = matchConverter(matchable, sourceType, targetType);
+				if (converter != null) {
+					return converter;
+				}
+			}
+			if (logger.isTraceEnabled()) {
+				logger.trace("and indexed by [java.lang.Object]");
+			}							
+			return matchConverter(converters.get(Object.class), sourceType, targetType);				
 		}
 	}
 
-	private void addInterfaceHierarchy(Class<?> interfaceType, LinkedList<Class<?>> classQueue, LinkedList<Class<?>> attemptedInterfaces) {
-		if (!attemptedInterfaces.contains(interfaceType)) {
-			classQueue.addFirst(interfaceType);
-			attemptedInterfaces.add(interfaceType);
-			for (Class<?> inheritedInterface : interfaceType.getInterfaces()) {
-				addInterfaceHierarchy(inheritedInterface, classQueue, attemptedInterfaces);
-			}
+	private void addInterfaceHierarchy(Class<?> interfaceType, Set<Class<?>> interfaces) {
+		interfaces.add(interfaceType);
+		for (Class<?> inheritedInterface : interfaceType.getInterfaces()) {
+			addInterfaceHierarchy(inheritedInterface, interfaces);
 		}
 	}
 

org.springframework.core/src/test/java/org/springframework/core/convert/TypeDescriptorTests.java

@@ -19,8 +19,10 @@ package org.springframework.core.convert;
 import static junit.framework.Assert.assertEquals;
 import static junit.framework.Assert.assertTrue;
 import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNull;
 
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Date;
 import java.util.HashMap;
 import java.util.List;
@@ -49,6 +51,110 @@ public class TypeDescriptorTests {
 	public Map<String, Integer> mapField = new HashMap<String, Integer>();
 
 	public Map<String, List<Integer>> nestedMapField = new HashMap<String, List<Integer>>();
+
+	@Test
+	public void forCollection() {
+		List<String> list = new ArrayList<String>();
+		list.add("1");
+		TypeDescriptor desc = TypeDescriptor.forObject(list);
+		assertEquals(String.class, desc.getElementType());
+	}
+
+	@Test
+	public void forCollectionEmpty() {
+		List<String> list = new ArrayList<String>();
+		TypeDescriptor desc = TypeDescriptor.forObject(list);
+		assertNull(desc.getElementType());
+	}
+
+	@Test
+	public void forCollectionSuperClassCommonType() throws SecurityException, NoSuchFieldException {
+		List<Number> list = new ArrayList<Number>();
+		list.add(1);
+		list.add(2L);
+		TypeDescriptor desc = TypeDescriptor.forObject(list);
+		assertEquals(Number.class, desc.getElementType());
+	}
+
+	public List<Long> longs;
+	
+	@Test
+	public void forCollectionNoObviousCommonType() {
+		List<Object> collection = new ArrayList<Object>();
+		List<String> list = new ArrayList<String>();
+		list.add("1");
+		collection.add(list);
+		Map<String, String> map = new HashMap<String, String>();
+		collection.add(map);
+		map.put("1", "2");
+		TypeDescriptor desc = TypeDescriptor.forObject(collection);
+		assertEquals(Cloneable.class, desc.getElementType());
+	}
+
+	@Test
+	public void forCollectionNoCommonType() {
+		List<Object> collection = new ArrayList<Object>();
+		collection.add(new Object());
+		collection.add("1");
+		TypeDescriptor desc = TypeDescriptor.forObject(collection);		
+		assertEquals(Object.class, desc.getElementType());
+	}
+
+	@Test
+	public void forCollectionNested() {
+		List<Object> collection = new ArrayList<Object>();
+		collection.add(Arrays.asList("1", "2"));
+		collection.add(Arrays.asList("3", "4"));
+		TypeDescriptor desc = TypeDescriptor.forObject(collection);
+		assertEquals(Arrays.asList("foo").getClass(), desc.getElementType());
+		assertEquals(String.class, desc.getElementTypeDescriptor().getElementType());
+	}
+
+	@Test
+	public void forMap() {
+		Map<String, String> map = new HashMap<String, String>();
+		map.put("1", "2");
+		TypeDescriptor desc = TypeDescriptor.forObject(map);
+		assertEquals(String.class, desc.getMapKeyType());
+		assertEquals(String.class, desc.getMapValueType());
+	}
+
+	@Test
+	public void forMapEmpty() {
+		Map<String, String> map = new HashMap<String, String>();
+		TypeDescriptor desc = TypeDescriptor.forObject(map);
+		assertNull(desc.getMapKeyType());
+		assertNull(desc.getMapValueType());
+	}
+
+	@Test
+	public void forMapCommonSuperClass() {
+		Map<Number, Number> map = new HashMap<Number, Number>();
+		map.put(1, 2);
+		map.put(2L, 3L);
+		TypeDescriptor desc = TypeDescriptor.forObject(map);
+		assertEquals(Number.class, desc.getMapKeyType());
+		assertEquals(Number.class, desc.getMapValueType());
+	}
+
+	@Test
+	public void forMapNoObviousCommonType() {
+		Map<Object, Object> map = new HashMap<Object, Object>();
+		map.put("1", "2");
+		map.put(2, 2);
+		TypeDescriptor desc = TypeDescriptor.forObject(map);
+		assertEquals(Comparable.class, desc.getMapKeyType());
+		assertEquals(Comparable.class, desc.getMapValueType());
+	}
+
+	@Test
+	public void forMapNested() {
+		Map<Integer, List<String>> map = new HashMap<Integer, List<String>>();
+		map.put(1, Arrays.asList("1, 2"));
+		TypeDescriptor desc = TypeDescriptor.forObject(map);
+		assertEquals(Integer.class, desc.getMapKeyType());
+		assertEquals(String.class, desc.getMapValueTypeDescriptor().getElementType());		
+	}
 	
 	@Test
 	public void listDescriptor() throws Exception {

org.springframework.core/src/test/java/org/springframework/core/convert/support/CollectionToCollectionConverterTests.java

@@ -48,7 +48,7 @@ public class CollectionToCollectionConverterTests {
 		assertEquals((Integer) 37, result.get(1));
 	}
 	
-	public List<Integer> scalarListTarget;
+	public ArrayList<Integer> scalarListTarget;
 
 	@Test
 	public void emptyListToList() throws Exception {