From aa914497dc312568cc541e53302038729364ce68 Mon Sep 17 00:00:00 2001 From: Phillip Webb Date: Sat, 22 Dec 2012 10:24:33 -0800 Subject: [PATCH] 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 --- .../support/GenericConversionService.java | 86 ++++++++----------- .../support/DefaultConversionTests.java | 34 ++++++-- .../GenericConversionServiceTests.java | 15 ++-- 3 files changed, 71 insertions(+), 64 deletions(-) diff --git a/spring-core/src/main/java/org/springframework/core/convert/support/GenericConversionService.java b/spring-core/src/main/java/org/springframework/core/convert/support/GenericConversionService.java index 3e69ee4421..f2267fcf3c 100644 --- a/spring-core/src/main/java/org/springframework/core/convert/support/GenericConversionService.java +++ b/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> IGNORED_CLASSES; - static { - Set> ignored = new HashSet>(); - ignored.add(Object.class); - ignored.add(Object[].class); - IGNORED_CLASSES = Collections.unmodifiableSet(ignored); - } - private final Set globalConverters = new LinkedHashSet(); @@ -483,12 +475,13 @@ public class GenericConversionService implements ConfigurableConversionService { */ public GenericConverter find(TypeDescriptor sourceType, TypeDescriptor targetType) { // Search the full type hierarchy - List sourceCandidates = getTypeHierarchy(sourceType); - List targetCandidates = getTypeHierarchy(targetType); - for (TypeDescriptor sourceCandidate : sourceCandidates) { - for (TypeDescriptor targetCandidate : targetCandidates) { + List> sourceCandidates = getClassHierarchy(sourceType.getType()); + List> targetCandidates = getClassHierarchy(targetType.getType()); + for (Class sourceCandidate : sourceCandidates) { + 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, - TypeDescriptor sourceCandidate, TypeDescriptor targetCandidate) { + private GenericConverter getRegisteredConverter(TypeDescriptor sourceType, + TypeDescriptor targetType, ConvertiblePair convertiblePair) { // Check specifically registered converters - ConvertersForPair convertersForPair = converters.get(new ConvertiblePair( - sourceCandidate.getType(), targetCandidate.getType())); + ConvertersForPair convertersForPair = converters.get(convertiblePair); 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,44 +518,38 @@ public class GenericConversionService implements ConfigurableConversionService { * @return an ordered list of all classes that the given type extends or * implements. */ - private List getTypeHierarchy(TypeDescriptor type) { - if(type.isPrimitive()) { - type = TypeDescriptor.valueOf(type.getObjectType()); - } - Set typeHierarchy = new LinkedHashSet(); - collectTypeHierarchy(typeHierarchy, type); - if(type.isArray()) { - typeHierarchy.add(TypeDescriptor.valueOf(Object[].class)); - } - typeHierarchy.add(TypeDescriptor.valueOf(Object.class)); - return new ArrayList(typeHierarchy); - } - - private void collectTypeHierarchy(Set typeHierarchy, - TypeDescriptor type) { - if(type != null && !IGNORED_CLASSES.contains(type.getType())) { - if(typeHierarchy.add(type)) { - Class superclass = type.getType().getSuperclass(); - if (type.isArray()) { - superclass = ClassUtils.resolvePrimitiveIfNecessary(superclass); - } - collectTypeHierarchy(typeHierarchy, createRelated(type, superclass)); - - for (Class implementsInterface : type.getType().getInterfaces()) { - collectTypeHierarchy(typeHierarchy, createRelated(type, implementsInterface)); - } + private List> getClassHierarchy(Class type) { + List> hierarchy = new ArrayList>(20); + Set> visited = new HashSet>(20); + addToClassHierarchy(0, ClassUtils.resolvePrimitiveIfNecessary(type), false, hierarchy, visited); + boolean array = type.isArray(); + int i = 0; + while (i < hierarchy.size()) { + Class candidate = hierarchy.get(i); + candidate = (array ? candidate.getComponentType() + : ClassUtils.resolvePrimitiveIfNecessary(candidate)); + Class superclass = candidate.getSuperclass(); + if (candidate.getSuperclass() != null && superclass != Object.class) { + addToClassHierarchy(i + 1, candidate.getSuperclass(), array, hierarchy, visited); } + for (Class implementedInterface : candidate.getInterfaces()) { + addToClassHierarchy(hierarchy.size(), implementedInterface, array, hierarchy, visited); + } + i++; } + addToClassHierarchy(hierarchy.size(), Object.class, array, hierarchy, visited); + addToClassHierarchy(hierarchy.size(), Object.class, false, hierarchy, visited); + return hierarchy; } - private TypeDescriptor createRelated(TypeDescriptor type, Class relatedType) { - if (relatedType == null && type.isArray()) { - relatedType = Array.newInstance(relatedType, 0).getClass(); + private void addToClassHierarchy(int index, Class type, boolean asArray, + List> hierarchy, Set> visited) { + if(asArray) { + type = Array.newInstance(type, 0).getClass(); } - if(!type.getType().equals(relatedType)) { - return type.upcast(relatedType); + if(visited.add(type)) { + hierarchy.add(index, type); } - return null; } @Override diff --git a/spring-core/src/test/java/org/springframework/core/convert/support/DefaultConversionTests.java b/spring-core/src/test/java/org/springframework/core/convert/support/DefaultConversionTests.java index e5d7e27d3a..e1fdb3dee0 100644 --- a/spring-core/src/test/java/org/springframework/core/convert/support/DefaultConversionTests.java +++ b/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 junit.framework.Assert.assertTrue; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertNull; -import static org.junit.Assert.assertSame; +import static org.junit.Assert.*; 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 source = new ArrayList(); + source.add("A"); + source.add("B"); + conversionService.addConverter(new Converter() { + @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 result = (List) 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; + } + } + } diff --git a/spring-core/src/test/java/org/springframework/core/convert/support/GenericConversionServiceTests.java b/spring-core/src/test/java/org/springframework/core/convert/support/GenericConversionServiceTests.java index fc2af6d87d..b5ede248ed 100644 --- a/spring-core/src/test/java/org/springframework/core/convert/support/GenericConversionServiceTests.java +++ b/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 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(); + while(iterator.hasNext()) { + 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 sourceTypes = new LinkedHashSet(); + private List sourceTypes = new ArrayList(); public Set getConvertibleTypes() { return null; @@ -800,7 +797,7 @@ public class GenericConversionServiceTests { return null; } - public Set getSourceTypes() { + public List getSourceTypes() { return sourceTypes; } }