From f43d0e100373abffd8e638ae9541a9bdc91dd7fc Mon Sep 17 00:00:00 2001 From: Keith Donald Date: Tue, 24 May 2011 22:20:54 +0000 Subject: [PATCH] Revised converter search algorithm to favor super classes before interface hierarchy --- .../core/convert/TypeDescriptor.java | 42 +++--- .../support/GenericConversionService.java | 142 ++++++++++++------ .../core/convert/TypeDescriptorTests.java | 106 +++++++++++++ .../CollectionToCollectionConverterTests.java | 2 +- 4 files changed, 225 insertions(+), 67 deletions(-) diff --git a/org.springframework.core/src/main/java/org/springframework/core/convert/TypeDescriptor.java b/org.springframework.core/src/main/java/org/springframework/core/convert/TypeDescriptor.java index 9d1e490e591..33416e8f166 100644 --- a/org.springframework.core/src/main/java/org/springframework/core/convert/TypeDescriptor.java +++ b/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> interfaces = new LinkedHashSet>(); LinkedList> classQueue = new LinkedList>(); 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); + } } - throw new IllegalStateException("Should never be invoked"); + for (Class interfaceType : interfaces) { + if (interfaceType.isAssignableFrom(valueClass)) { + return interfaceType; + } + } + return Object.class; } - - private static void addInterfaceHierarchy(Class ifc, LinkedList> classQueue) { - classQueue.addFirst(ifc); - for (Class inheritedIfc : ifc.getInterfaces()) { - addInterfaceHierarchy(inheritedIfc, classQueue); + + private static void addInterfaceHierarchy(Class interfaceType, Set> 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 { } } - + } \ No newline at end of file diff --git a/org.springframework.core/src/main/java/org/springframework/core/convert/support/GenericConversionService.java b/org.springframework.core/src/main/java/org/springframework/core/convert/support/GenericConversionService.java index 2645bfa2d51..74346ecbe39 100644 --- a/org.springframework.core/src/main/java/org/springframework/core/convert/support/GenericConversionService.java +++ b/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, MatchableConverters> converters = getTargetConvertersForSource(currentClass); GenericConverter converter = getMatchingConverterForTarget(sourceType, targetType, converters); @@ -349,40 +350,62 @@ public class GenericConversionService implements ConversionService, ConverterReg Map, MatchableConverters> objectConverters = getTargetConvertersForSource(Object.class); return getMatchingConverterForTarget(sourceType, targetType, objectConverters); } - else { + else if (sourceObjectType.isArray()) { LinkedList> classQueue = new LinkedList>(); classQueue.addFirst(sourceObjectType); - LinkedList> attemptedInterfaces = new LinkedList>(); 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, 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> interfaces = new LinkedHashSet>(); + LinkedList> classQueue = new LinkedList>(); + 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, 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, MatchableConverters> converters = getTargetConvertersForSource(interfaceType); + GenericConverter converter = getMatchingConverterForTarget(sourceType, targetType, converters); + if (converter != null) { + return converter; + } + } + Map, 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 { + } else if (targetObjectType.isArray()) { LinkedList> classQueue = new LinkedList>(); classQueue.addFirst(targetObjectType); - LinkedList> attemptedInterfaces = new LinkedList>(); while (!classQueue.isEmpty()) { Class currentClass = classQueue.removeLast(); if (logger.isTraceEnabled()) { - logger.trace("and indexed by targetType [" + currentClass.getName() + "]"); + 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; } - 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 { + Set> interfaces = new LinkedHashSet>(); + LinkedList> classQueue = new LinkedList>(); + classQueue.addFirst(targetObjectType); + while (!classQueue.isEmpty()) { + Class currentClass = classQueue.removeLast(); + if (logger.isTraceEnabled()) { + 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; + } + 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("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> classQueue, LinkedList> 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> interfaces) { + interfaces.add(interfaceType); + for (Class inheritedInterface : interfaceType.getInterfaces()) { + addInterfaceHierarchy(inheritedInterface, interfaces); } } diff --git a/org.springframework.core/src/test/java/org/springframework/core/convert/TypeDescriptorTests.java b/org.springframework.core/src/test/java/org/springframework/core/convert/TypeDescriptorTests.java index 37d5a214d80..7ad044dc21f 100644 --- a/org.springframework.core/src/test/java/org/springframework/core/convert/TypeDescriptorTests.java +++ b/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 mapField = new HashMap(); public Map> nestedMapField = new HashMap>(); + + @Test + public void forCollection() { + List list = new ArrayList(); + list.add("1"); + TypeDescriptor desc = TypeDescriptor.forObject(list); + assertEquals(String.class, desc.getElementType()); + } + + @Test + public void forCollectionEmpty() { + List list = new ArrayList(); + TypeDescriptor desc = TypeDescriptor.forObject(list); + assertNull(desc.getElementType()); + } + + @Test + public void forCollectionSuperClassCommonType() throws SecurityException, NoSuchFieldException { + List list = new ArrayList(); + list.add(1); + list.add(2L); + TypeDescriptor desc = TypeDescriptor.forObject(list); + assertEquals(Number.class, desc.getElementType()); + } + + public List longs; + + @Test + public void forCollectionNoObviousCommonType() { + List collection = new ArrayList(); + List list = new ArrayList(); + list.add("1"); + collection.add(list); + Map map = new HashMap(); + collection.add(map); + map.put("1", "2"); + TypeDescriptor desc = TypeDescriptor.forObject(collection); + assertEquals(Cloneable.class, desc.getElementType()); + } + + @Test + public void forCollectionNoCommonType() { + List collection = new ArrayList(); + collection.add(new Object()); + collection.add("1"); + TypeDescriptor desc = TypeDescriptor.forObject(collection); + assertEquals(Object.class, desc.getElementType()); + } + + @Test + public void forCollectionNested() { + List collection = new ArrayList(); + 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 map = new HashMap(); + map.put("1", "2"); + TypeDescriptor desc = TypeDescriptor.forObject(map); + assertEquals(String.class, desc.getMapKeyType()); + assertEquals(String.class, desc.getMapValueType()); + } + + @Test + public void forMapEmpty() { + Map map = new HashMap(); + TypeDescriptor desc = TypeDescriptor.forObject(map); + assertNull(desc.getMapKeyType()); + assertNull(desc.getMapValueType()); + } + + @Test + public void forMapCommonSuperClass() { + Map map = new HashMap(); + 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 map = new HashMap(); + 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> map = new HashMap>(); + 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 { diff --git a/org.springframework.core/src/test/java/org/springframework/core/convert/support/CollectionToCollectionConverterTests.java b/org.springframework.core/src/test/java/org/springframework/core/convert/support/CollectionToCollectionConverterTests.java index d6eb5596d88..668208cb1a7 100644 --- a/org.springframework.core/src/test/java/org/springframework/core/convert/support/CollectionToCollectionConverterTests.java +++ b/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 scalarListTarget; + public ArrayList scalarListTarget; @Test public void emptyListToList() throws Exception {