From e943fdeae4e7d5d785607c26a30170b6895fa7cc Mon Sep 17 00:00:00 2001 From: Oliver Gierke Date: Wed, 12 Nov 2014 14:30:34 +0100 Subject: [PATCH] DATACMNS-590 - Fixed calculation of nested generics in ParentTypeAwareTypeInformation. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit So far, the lookup of the type variable map was preferring the type variable maps detected on parent types in nested structures. This caused the concrete type variables in nested types not being considered correctly which caused the type resolution to fall back to the generic bounds. We now accumulate the type variable maps to avoid having to lookup a certain map in the nesting hierarchy. The core fix is in ParentTypeAwareTypeInformation's constructor and mergeMaps(…) respectively. Simplified the handling of type variable maps and made proper use of generics throughout the class hierarchy. --- .../data/util/ClassTypeInformation.java | 24 +++++++-- .../util/GenericArrayTypeInformation.java | 14 ++++-- .../util/ParameterizedTypeInformation.java | 13 +++-- .../util/ParentTypeAwareTypeInformation.java | 28 ++++++----- .../data/util/TypeDiscoverer.java | 30 +++++++----- .../util/TypeVariableTypeInformation.java | 5 +- .../util/ClassTypeInformationUnitTests.java | 36 +++++++++++++- .../data/util/ParameterizedTypeUnitTests.java | 23 +++++---- .../data/util/TypeDiscovererUnitTests.java | 49 ++++++++++++------- 9 files changed, 152 insertions(+), 70 deletions(-) diff --git a/src/main/java/org/springframework/data/util/ClassTypeInformation.java b/src/main/java/org/springframework/data/util/ClassTypeInformation.java index afecda03a..a3caf9d8b 100644 --- a/src/main/java/org/springframework/data/util/ClassTypeInformation.java +++ b/src/main/java/org/springframework/data/util/ClassTypeInformation.java @@ -23,8 +23,10 @@ import java.lang.reflect.TypeVariable; import java.util.Arrays; import java.util.Collection; import java.util.Collections; +import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Map.Entry; import java.util.Set; import java.util.WeakHashMap; @@ -98,12 +100,26 @@ public class ClassTypeInformation extends TypeDiscoverer { * @param type */ ClassTypeInformation(Class type) { - this(type, GenericTypeResolver.getTypeVariableMap(type)); + super(ClassUtils.getUserClass(type), getTypeVariableMap(type)); + this.type = type; } - ClassTypeInformation(Class type, Map typeVariableMap) { - super(ClassUtils.getUserClass(type), typeVariableMap); - this.type = type; + /** + * Little helper to allow us to create a generified map, actually just to satisfy the compiler. + * + * @param type must not be {@literal null}. + * @return + */ + private static Map, Type> getTypeVariableMap(Class type) { + + Map source = GenericTypeResolver.getTypeVariableMap(type); + Map, Type> map = new HashMap, Type>(source.size()); + + for (Entry entry : source.entrySet()) { + map.put(entry.getKey(), entry.getValue()); + } + + return map; } /* diff --git a/src/main/java/org/springframework/data/util/GenericArrayTypeInformation.java b/src/main/java/org/springframework/data/util/GenericArrayTypeInformation.java index 1eeacf7f0..9f91b123f 100644 --- a/src/main/java/org/springframework/data/util/GenericArrayTypeInformation.java +++ b/src/main/java/org/springframework/data/util/GenericArrayTypeInformation.java @@ -18,6 +18,8 @@ package org.springframework.data.util; import java.lang.reflect.Array; import java.lang.reflect.GenericArrayType; import java.lang.reflect.Type; +import java.lang.reflect.TypeVariable; +import java.util.Map; /** * Special {@link TypeDiscoverer} handling {@link GenericArrayType}s. @@ -26,18 +28,20 @@ import java.lang.reflect.Type; */ class GenericArrayTypeInformation extends ParentTypeAwareTypeInformation { - private GenericArrayType type; + private final GenericArrayType type; /** * Creates a new {@link GenericArrayTypeInformation} for the given {@link GenericArrayTypeInformation} and * {@link TypeDiscoverer}. * - * @param type - * @param parent + * @param type must not be {@literal null}. + * @param parent must not be {@literal null}. + * @param typeVariableMap must not be {@literal null}. */ - protected GenericArrayTypeInformation(GenericArrayType type, TypeDiscoverer parent) { + protected GenericArrayTypeInformation(GenericArrayType type, TypeDiscoverer parent, + Map, Type> typeVariableMap) { - super(type, parent, parent.getTypeVariableMap()); + super(type, parent, typeVariableMap); this.type = type; } diff --git a/src/main/java/org/springframework/data/util/ParameterizedTypeInformation.java b/src/main/java/org/springframework/data/util/ParameterizedTypeInformation.java index 3c301ade3..acae86349 100644 --- a/src/main/java/org/springframework/data/util/ParameterizedTypeInformation.java +++ b/src/main/java/org/springframework/data/util/ParameterizedTypeInformation.java @@ -17,6 +17,7 @@ package org.springframework.data.util; import java.lang.reflect.ParameterizedType; import java.lang.reflect.Type; +import java.lang.reflect.TypeVariable; import java.util.ArrayList; import java.util.Arrays; import java.util.HashSet; @@ -24,7 +25,6 @@ import java.util.List; import java.util.Map; import java.util.Set; -import org.springframework.core.GenericTypeResolver; import org.springframework.util.StringUtils; /** @@ -44,8 +44,10 @@ class ParameterizedTypeInformation extends ParentTypeAwareTypeInformation * @param type must not be {@literal null} * @param parent must not be {@literal null} */ - public ParameterizedTypeInformation(ParameterizedType type, TypeDiscoverer parent) { - super(type, parent, null); + public ParameterizedTypeInformation(ParameterizedType type, TypeDiscoverer parent, + Map, Type> typeVariableMap) { + + super(type, parent, typeVariableMap); this.type = type; } @@ -72,8 +74,11 @@ class ParameterizedTypeInformation extends ParentTypeAwareTypeInformation supertypes.addAll(Arrays.asList(rawType.getGenericInterfaces())); for (Type supertype : supertypes) { - Class rawSuperType = GenericTypeResolver.resolveType(supertype, getTypeVariableMap()); + + Class rawSuperType = resolveType(supertype); + if (Map.class.isAssignableFrom(rawSuperType)) { + ParameterizedType parameterizedSupertype = (ParameterizedType) supertype; Type[] arguments = parameterizedSupertype.getActualTypeArguments(); return createInfo(arguments[1]); diff --git a/src/main/java/org/springframework/data/util/ParentTypeAwareTypeInformation.java b/src/main/java/org/springframework/data/util/ParentTypeAwareTypeInformation.java index 0411b1e69..8fb4bc093 100644 --- a/src/main/java/org/springframework/data/util/ParentTypeAwareTypeInformation.java +++ b/src/main/java/org/springframework/data/util/ParentTypeAwareTypeInformation.java @@ -17,6 +17,7 @@ package org.springframework.data.util; import java.lang.reflect.Type; import java.lang.reflect.TypeVariable; +import java.util.HashMap; import java.util.Map; import org.springframework.util.ObjectUtils; @@ -33,27 +34,30 @@ public abstract class ParentTypeAwareTypeInformation extends TypeDiscoverer parent, Map map) { - - super(type, map); + protected ParentTypeAwareTypeInformation(Type type, TypeDiscoverer parent, Map, Type> map) { + super(type, mergeMaps(parent, map)); this.parent = parent; } /** - * Considers the parent's type variable map before invoking the super class method. + * Merges the type variable maps of the given parent with the new map. * + * @param parent must not be {@literal null}. + * @param map must not be {@literal null}. * @return */ - @Override - @SuppressWarnings("rawtypes") - protected Map getTypeVariableMap() { - return parent == null ? super.getTypeVariableMap() : parent.getTypeVariableMap(); + private static Map, Type> mergeMaps(TypeDiscoverer parent, Map, Type> map) { + + Map, Type> typeVariableMap = new HashMap, Type>(); + typeVariableMap.putAll(map); + typeVariableMap.putAll(parent.getTypeVariableMap()); + + return typeVariableMap; } /* diff --git a/src/main/java/org/springframework/data/util/TypeDiscoverer.java b/src/main/java/org/springframework/data/util/TypeDiscoverer.java index c43b3407b..1f77cc84f 100644 --- a/src/main/java/org/springframework/data/util/TypeDiscoverer.java +++ b/src/main/java/org/springframework/data/util/TypeDiscoverer.java @@ -30,6 +30,7 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; import java.util.Collections; +import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.concurrent.ConcurrentHashMap; @@ -47,7 +48,7 @@ import org.springframework.util.ReflectionUtils; class TypeDiscoverer implements TypeInformation { private final Type type; - @SuppressWarnings("rawtypes") private final Map typeVariableMap; + private final Map, Type> typeVariableMap; private final Map> fieldTypes = new ConcurrentHashMap>(); private Class resolvedType; @@ -55,25 +56,24 @@ class TypeDiscoverer implements TypeInformation { /** * Creates a ne {@link TypeDiscoverer} for the given type, type variable map and parent. * - * @param type must not be null. - * @param typeVariableMap + * @param type must not be {@literal null}. + * @param typeVariableMap must not be {@literal null}. */ - @SuppressWarnings("rawtypes") - protected TypeDiscoverer(Type type, Map typeVariableMap) { + protected TypeDiscoverer(Type type, Map, Type> typeVariableMap) { Assert.notNull(type); + Assert.notNull(typeVariableMap); + this.type = type; this.typeVariableMap = typeVariableMap; } /** - * Returns the type variable map. Will traverse the parents up to the root on and use it's map. + * Returns the type variable map. * * @return */ - @SuppressWarnings("rawtypes") - protected Map getTypeVariableMap() { - + protected Map, Type> getTypeVariableMap() { return typeVariableMap; } @@ -98,7 +98,7 @@ class TypeDiscoverer implements TypeInformation { if (fieldType instanceof ParameterizedType) { ParameterizedType parameterizedType = (ParameterizedType) fieldType; - return new ParameterizedTypeInformation(parameterizedType, this); + return new ParameterizedTypeInformation(parameterizedType, this, variableMap); } if (fieldType instanceof TypeVariable) { @@ -107,7 +107,7 @@ class TypeDiscoverer implements TypeInformation { } if (fieldType instanceof GenericArrayType) { - return new GenericArrayTypeInformation((GenericArrayType) fieldType, this); + return new GenericArrayTypeInformation((GenericArrayType) fieldType, this, variableMap); } if (fieldType instanceof WildcardType) { @@ -135,9 +135,13 @@ class TypeDiscoverer implements TypeInformation { * @param type * @return */ - @SuppressWarnings("unchecked") + @SuppressWarnings({ "unchecked", "rawtypes" }) protected Class resolveType(Type type) { - return (Class) GenericTypeResolver.resolveType(type, getTypeVariableMap()); + + Map map = new HashMap(); + map.putAll(getTypeVariableMap()); + + return (Class) GenericTypeResolver.resolveType(type, map); } /* diff --git a/src/main/java/org/springframework/data/util/TypeVariableTypeInformation.java b/src/main/java/org/springframework/data/util/TypeVariableTypeInformation.java index 794c7ae31..ec36f9071 100644 --- a/src/main/java/org/springframework/data/util/TypeVariableTypeInformation.java +++ b/src/main/java/org/springframework/data/util/TypeVariableTypeInformation.java @@ -43,11 +43,10 @@ class TypeVariableTypeInformation extends ParentTypeAwareTypeInformation { * @param owningType must not be {@literal null} * @param parent */ - @SuppressWarnings("rawtypes") public TypeVariableTypeInformation(TypeVariable variable, Type owningType, TypeDiscoverer parent, - Map map) { + Map, Type> typeVariableMap) { - super(variable, parent, map); + super(variable, parent, typeVariableMap); Assert.notNull(variable); this.variable = variable; this.owningType = owningType; diff --git a/src/test/java/org/springframework/data/util/ClassTypeInformationUnitTests.java b/src/test/java/org/springframework/data/util/ClassTypeInformationUnitTests.java index 9459797f8..d498afc16 100644 --- a/src/test/java/org/springframework/data/util/ClassTypeInformationUnitTests.java +++ b/src/test/java/org/springframework/data/util/ClassTypeInformationUnitTests.java @@ -93,7 +93,7 @@ public class ClassTypeInformationUnitTests { TypeInformation information = ClassTypeInformation.from(StringCollectionContainer.class); TypeInformation property = information.getProperty("array"); - assertEquals(property.getComponentType().getType(), String.class); + assertThat(property.getComponentType().getType(), is((Object) String.class)); Class type = property.getType(); assertEquals(String[].class, type); @@ -327,6 +327,20 @@ public class ClassTypeInformationUnitTests { is("org.springframework.data.util.ClassTypeInformationUnitTests$SpecialPerson")); } + /** + * @see DATACMNS-590 + */ + @Test + public void resolvesNestedGenericsToConcreteType() { + + ClassTypeInformation rootType = from(ConcreteRoot.class); + TypeInformation subsPropertyType = rootType.getProperty("subs"); + TypeInformation subsElementType = subsPropertyType.getActualType(); + TypeInformation subSubType = subsElementType.getProperty("subSub"); + + assertThat(subSubType.getType(), is((Object) ConcreteSubSub.class)); + } + static class StringMapContainer extends MapContainer { } @@ -461,4 +475,24 @@ public class ClassTypeInformationUnitTests { SortedMap>> seriously; } + + // DATACMNS-590 + + static abstract class GenericRoot> { + List subs; + } + + static abstract class GenericSub { + T subSub; + } + + static abstract class GenericSubSub {} + + static class ConcreteRoot extends GenericRoot {} + + static class ConcreteSub extends GenericSub {} + + static class ConcreteSubSub extends GenericSubSub { + String content; + } } diff --git a/src/test/java/org/springframework/data/util/ParameterizedTypeUnitTests.java b/src/test/java/org/springframework/data/util/ParameterizedTypeUnitTests.java index 3f9222a11..1eae5bd99 100644 --- a/src/test/java/org/springframework/data/util/ParameterizedTypeUnitTests.java +++ b/src/test/java/org/springframework/data/util/ParameterizedTypeUnitTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2011 the original author or authors. + * Copyright 2011-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. @@ -22,8 +22,11 @@ import static org.springframework.data.util.ClassTypeInformation.*; import java.lang.reflect.ParameterizedType; import java.lang.reflect.Type; +import java.lang.reflect.TypeVariable; +import java.util.Collections; import java.util.HashMap; import java.util.Locale; +import java.util.Map; import org.junit.Before; import org.junit.Test; @@ -39,6 +42,8 @@ import org.mockito.runners.MockitoJUnitRunner; @RunWith(MockitoJUnitRunner.class) public class ParameterizedTypeUnitTests { + static final Map, Type> EMPTY_MAP = Collections.emptyMap(); + @Mock ParameterizedType one; @Before @@ -49,22 +54,22 @@ public class ParameterizedTypeUnitTests { @Test public void considersTypeInformationsWithDifferingParentsNotEqual() { - TypeDiscoverer stringParent = new TypeDiscoverer(String.class, null); - TypeDiscoverer objectParent = new TypeDiscoverer(Object.class, null); + TypeDiscoverer stringParent = new TypeDiscoverer(String.class, EMPTY_MAP); + TypeDiscoverer objectParent = new TypeDiscoverer(Object.class, EMPTY_MAP); - ParameterizedTypeInformation first = new ParameterizedTypeInformation(one, stringParent); - ParameterizedTypeInformation second = new ParameterizedTypeInformation(one, objectParent); + ParameterizedTypeInformation first = new ParameterizedTypeInformation(one, stringParent, EMPTY_MAP); + ParameterizedTypeInformation second = new ParameterizedTypeInformation(one, objectParent, EMPTY_MAP); - assertFalse(first.equals(second)); + assertThat(first, is(not(second))); } @Test public void considersTypeInformationsWithSameParentsNotEqual() { - TypeDiscoverer stringParent = new TypeDiscoverer(String.class, null); + TypeDiscoverer stringParent = new TypeDiscoverer(String.class, EMPTY_MAP); - ParameterizedTypeInformation first = new ParameterizedTypeInformation(one, stringParent); - ParameterizedTypeInformation second = new ParameterizedTypeInformation(one, stringParent); + ParameterizedTypeInformation first = new ParameterizedTypeInformation(one, stringParent, EMPTY_MAP); + ParameterizedTypeInformation second = new ParameterizedTypeInformation(one, stringParent, EMPTY_MAP); assertTrue(first.equals(second)); } diff --git a/src/test/java/org/springframework/data/util/TypeDiscovererUnitTests.java b/src/test/java/org/springframework/data/util/TypeDiscovererUnitTests.java index 5b4a034cd..59800c888 100644 --- a/src/test/java/org/springframework/data/util/TypeDiscovererUnitTests.java +++ b/src/test/java/org/springframework/data/util/TypeDiscovererUnitTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2011-2012 the original author or authors. + * Copyright 2011-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. @@ -17,11 +17,13 @@ package org.springframework.data.util; import static org.hamcrest.CoreMatchers.*; import static org.junit.Assert.*; +import static org.springframework.data.util.ClassTypeInformation.*; import java.lang.reflect.Constructor; import java.lang.reflect.Type; import java.lang.reflect.TypeVariable; import java.util.Collection; +import java.util.Collections; import java.util.List; import java.util.Locale; import java.util.Map; @@ -39,13 +41,10 @@ import org.mockito.runners.MockitoJUnitRunner; @RunWith(MockitoJUnitRunner.class) public class TypeDiscovererUnitTests { - @Mock - @SuppressWarnings("rawtypes") - Map firstMap; + static final Map, Type> EMPTY_MAP = Collections.emptyMap(); - @Mock - @SuppressWarnings("rawtypes") - Map secondMap; + @Mock Map, Type> firstMap; + @Mock Map, Type> secondMap; @Test(expected = IllegalArgumentException.class) public void rejectsNullType() { @@ -55,8 +54,8 @@ public class TypeDiscovererUnitTests { @Test public void isNotEqualIfTypesDiffer() { - TypeDiscoverer objectTypeInfo = new TypeDiscoverer(Object.class, null); - TypeDiscoverer stringTypeInfo = new TypeDiscoverer(String.class, null); + TypeDiscoverer objectTypeInfo = new TypeDiscoverer(Object.class, EMPTY_MAP); + TypeDiscoverer stringTypeInfo = new TypeDiscoverer(String.class, EMPTY_MAP); assertFalse(objectTypeInfo.equals(stringTypeInfo)); } @@ -75,37 +74,44 @@ public class TypeDiscovererUnitTests { @Test public void dealsWithTypesReferencingThemselves() { - TypeInformation information = new ClassTypeInformation(SelfReferencing.class); + TypeInformation information = from(SelfReferencing.class); TypeInformation first = information.getProperty("parent").getMapValueType(); TypeInformation second = first.getProperty("map").getMapValueType(); + assertEquals(first, second); } @Test public void dealsWithTypesReferencingThemselvesInAMap() { - TypeInformation information = new ClassTypeInformation( - SelfReferencingMap.class); + TypeInformation information = from(SelfReferencingMap.class); TypeInformation mapValueType = information.getProperty("map").getMapValueType(); + assertEquals(mapValueType, information); } @Test public void returnsComponentAndValueTypesForMapExtensions() { - TypeInformation discoverer = new TypeDiscoverer(CustomMap.class, null); + + TypeInformation discoverer = new TypeDiscoverer(CustomMap.class, EMPTY_MAP); + assertEquals(Locale.class, discoverer.getMapValueType().getType()); assertEquals(String.class, discoverer.getComponentType().getType()); } @Test public void returnsComponentTypeForCollectionExtension() { - TypeDiscoverer discoverer = new TypeDiscoverer(CustomCollection.class, null); + + TypeDiscoverer discoverer = new TypeDiscoverer(CustomCollection.class, firstMap); + assertEquals(String.class, discoverer.getComponentType().getType()); } @Test public void returnsComponentTypeForArrays() { - TypeDiscoverer discoverer = new TypeDiscoverer(String[].class, null); + + TypeDiscoverer discoverer = new TypeDiscoverer(String[].class, EMPTY_MAP); + assertEquals(String.class, discoverer.getComponentType().getType()); } @@ -117,9 +123,10 @@ public class TypeDiscovererUnitTests { public void discoveresConstructorParameterTypesCorrectly() throws NoSuchMethodException, SecurityException { TypeDiscoverer discoverer = new TypeDiscoverer(GenericConstructors.class, - null); + firstMap); Constructor constructor = GenericConstructors.class.getConstructor(List.class, Locale.class); List> types = discoverer.getParameterTypes(constructor); + assertThat(types.size(), is(2)); assertThat(types.get(0).getType(), equalTo((Class) List.class)); assertThat(types.get(0).getComponentType().getType(), is(equalTo((Class) String.class))); @@ -128,7 +135,9 @@ public class TypeDiscovererUnitTests { @Test @SuppressWarnings("rawtypes") public void returnsNullForComponentAndValueTypesForRawMaps() { - TypeDiscoverer discoverer = new TypeDiscoverer(Map.class, null); + + TypeDiscoverer discoverer = new TypeDiscoverer(Map.class, EMPTY_MAP); + assertThat(discoverer.getComponentType(), is(nullValue())); assertThat(discoverer.getMapValueType(), is(nullValue())); } @@ -140,14 +149,16 @@ public class TypeDiscovererUnitTests { @SuppressWarnings("rawtypes") public void doesNotConsiderTypeImplementingIterableACollection() { - TypeDiscoverer discoverer = new TypeDiscoverer(Person.class, null); - TypeInformation reference = ClassTypeInformation.from(Address.class); + TypeDiscoverer discoverer = new TypeDiscoverer(Person.class, EMPTY_MAP); + TypeInformation reference = from(Address.class); TypeInformation addresses = discoverer.getProperty("addresses"); + assertThat(addresses.isCollectionLike(), is(false)); assertThat(addresses.getComponentType(), is(reference)); TypeInformation adressIterable = discoverer.getProperty("addressIterable"); + assertThat(adressIterable.isCollectionLike(), is(true)); assertThat(adressIterable.getComponentType(), is(reference)); }