From baee26e5e727aa9328cd850d232bcea497b55647 Mon Sep 17 00:00:00 2001 From: Oliver Gierke Date: Fri, 27 Oct 2017 10:40:38 +0200 Subject: [PATCH] DATACMNS-1180 - Fixed accessor lookup for generic properties. In AbstractPersistentProperty, we now resolve the potentially generic return and parameter types of getters and setters. To achieve that Property has now been made aware of the actual owning type. --- .../context/AbstractMappingContext.java | 7 ++-- .../data/mapping/model/Property.java | 28 ++++++++----- .../AbstractPersistentPropertyUnitTests.java | 41 +++++++++++++++---- 3 files changed, 54 insertions(+), 22 deletions(-) diff --git a/src/main/java/org/springframework/data/mapping/context/AbstractMappingContext.java b/src/main/java/org/springframework/data/mapping/context/AbstractMappingContext.java index 218eb0342..0d3a6504c 100644 --- a/src/main/java/org/springframework/data/mapping/context/AbstractMappingContext.java +++ b/src/main/java/org/springframework/data/mapping/context/AbstractMappingContext.java @@ -513,12 +513,13 @@ public abstract class AbstractMappingContext type = entity.getTypeInformation(); ReflectionUtils.makeAccessible(field); Property property = Optional.ofNullable(descriptors.get(fieldName))// - .map(it -> Property.of(field, it))// - .orElseGet(() -> Property.of(field)); + .map(it -> Property.of(type, field, it))// + .orElseGet(() -> Property.of(type, field)); createAndRegisterProperty(property); @@ -535,7 +536,7 @@ public abstract class AbstractMappingContext Property.of(entity.getTypeInformation(), it)) // .filter(PersistentPropertyFilter.INSTANCE::matches) // .forEach(this::createAndRegisterProperty); } diff --git a/src/main/java/org/springframework/data/mapping/model/Property.java b/src/main/java/org/springframework/data/mapping/model/Property.java index 125c028e3..f8f7f6b74 100644 --- a/src/main/java/org/springframework/data/mapping/model/Property.java +++ b/src/main/java/org/springframework/data/mapping/model/Property.java @@ -26,6 +26,7 @@ import java.util.function.Function; import org.springframework.data.util.Lazy; import org.springframework.data.util.Optionals; +import org.springframework.data.util.TypeInformation; import org.springframework.lang.Nullable; import org.springframework.util.Assert; @@ -49,68 +50,75 @@ public class Property { private final Lazy name; private final Lazy toString; - private Property(Optional field, Optional descriptor) { + private Property(TypeInformation type, Optional field, Optional descriptor) { + Assert.notNull(type, "Type must not be null!"); Assert.isTrue(Optionals.isAnyPresent(field, descriptor), "Either field or descriptor has to be given!"); this.field = field; this.descriptor = descriptor; - this.rawType = withFieldOrDescriptor(Field::getType, PropertyDescriptor::getPropertyType); + this.rawType = withFieldOrDescriptor( // + it -> type.getRequiredProperty(it.getName()).getType(), // + it -> type.getRequiredProperty(it.getName()).getType() // + ); this.hashCode = Lazy.of(() -> withFieldOrDescriptor(Object::hashCode)); this.name = Lazy.of(() -> withFieldOrDescriptor(Field::getName, FeatureDescriptor::getName)); this.toString = Lazy.of(() -> withFieldOrDescriptor(Object::toString)); this.getter = descriptor.map(PropertyDescriptor::getReadMethod)// .filter(it -> getType() != null)// - .filter(it -> getType().isAssignableFrom(it.getReturnType())); + .filter(it -> getType().isAssignableFrom(type.getReturnType(it).getType())); this.setter = descriptor.map(PropertyDescriptor::getWriteMethod)// .filter(it -> getType() != null)// - .filter(it -> it.getParameterTypes()[0].isAssignableFrom(getType())); + .filter(it -> type.getParameterTypes(it).get(0).getType().isAssignableFrom(getType())); } /** * Creates a new {@link Property} backed by the given field. * + * @param type the owning type, must not be {@literal null}. * @param field must not be {@literal null}. * @return */ - public static Property of(Field field) { + public static Property of(TypeInformation type, Field field) { Assert.notNull(field, "Field must not be null!"); - return new Property(Optional.of(field), Optional.empty()); + return new Property(type, Optional.of(field), Optional.empty()); } /** * Creates a new {@link Property} backed by the given {@link Field} and {@link PropertyDescriptor}. * + * @param type the owning type, must not be {@literal null}. * @param field must not be {@literal null}. * @param descriptor must not be {@literal null}. * @return */ - public static Property of(Field field, PropertyDescriptor descriptor) { + public static Property of(TypeInformation type, Field field, PropertyDescriptor descriptor) { Assert.notNull(field, "Field must not be null!"); Assert.notNull(descriptor, "PropertyDescriptor must not be null!"); - return new Property(Optional.of(field), Optional.of(descriptor)); + return new Property(type, Optional.of(field), Optional.of(descriptor)); } /** * Creates a new {@link Property} for the given {@link PropertyDescriptor}. The creation might fail if the given * property is not representing a proper property. * + * @param type the owning type, must not be {@literal null}. * @param descriptor must not be {@literal null}. * @return * @see #supportsStandalone(PropertyDescriptor) */ - public static Property of(PropertyDescriptor descriptor) { + public static Property of(TypeInformation type, PropertyDescriptor descriptor) { Assert.notNull(descriptor, "PropertyDescriptor must not be null!"); - return new Property(Optional.empty(), Optional.of(descriptor)); + return new Property(type, Optional.empty(), Optional.of(descriptor)); } /** diff --git a/src/test/java/org/springframework/data/mapping/model/AbstractPersistentPropertyUnitTests.java b/src/test/java/org/springframework/data/mapping/model/AbstractPersistentPropertyUnitTests.java index 22c62c0e9..d0556dc8e 100755 --- a/src/test/java/org/springframework/data/mapping/model/AbstractPersistentPropertyUnitTests.java +++ b/src/test/java/org/springframework/data/mapping/model/AbstractPersistentPropertyUnitTests.java @@ -17,6 +17,9 @@ package org.springframework.data.mapping.model; import static org.assertj.core.api.Assertions.*; +import lombok.Getter; +import lombok.Setter; + import java.beans.IntrospectionException; import java.beans.Introspector; import java.beans.PropertyDescriptor; @@ -36,6 +39,7 @@ import org.springframework.data.mapping.PersistentEntity; import org.springframework.data.mapping.PersistentProperty; import org.springframework.data.mapping.Person; import org.springframework.data.util.ClassTypeInformation; +import org.springframework.data.util.Optionals; import org.springframework.data.util.TypeInformation; import org.springframework.util.ReflectionUtils; @@ -136,11 +140,13 @@ public class AbstractPersistentPropertyUnitTests { public void doesNotDiscoverGetterAndSetterIfNoPropertyDescriptorGiven() { Field field = ReflectionUtils.findField(AccessorTestClass.class, "id"); - PersistentProperty property = new SamplePersistentProperty(Property.of(field), + Property property = Property.of(ClassTypeInformation.from(AccessorTestClass.class), field); + + PersistentProperty persistentProperty = new SamplePersistentProperty(property, getEntity(AccessorTestClass.class), typeHolder); - assertThat(property.getGetter()).isNull(); - assertThat(property.getSetter()).isNull(); + assertThat(persistentProperty.getGetter()).isNull(); + assertThat(persistentProperty.getSetter()).isNull(); } @Test // DATACMNS-337 @@ -211,21 +217,30 @@ public class AbstractPersistentPropertyUnitTests { assertThat(property.getRawType()).isEqualTo(String.class); } + @Test // DATACMNS-1180 + public void returnsAccessorsForGenericReturnType() { + + SamplePersistentProperty property = getProperty(ConcreteGetter.class, "genericField"); + + assertThat(property.getSetter()).isNotNull(); + assertThat(property.getGetter()).isNotNull(); + } + private BasicPersistentEntity getEntity(Class type) { return new BasicPersistentEntity<>(ClassTypeInformation.from(type)); } private SamplePersistentProperty getProperty(Class type, String name) { + TypeInformation typeInformation = ClassTypeInformation.from(type); Optional field = Optional.ofNullable(ReflectionUtils.findField(type, name)); Optional descriptor = getPropertyDescriptor(type, name); - Property property = field.map(it -> descriptor// - .map(foo -> Property.of(it, foo))// - .orElseGet(() -> Property.of(it))).orElseGet(() -> getPropertyDescriptor(type, name)// - .map(it -> Property.of(it))// - .orElseThrow( - () -> new IllegalArgumentException(String.format("Couldn't find property %s on %s!", name, type)))); + Property property = Optionals.firstNonEmpty( // + () -> Optionals.mapIfAllPresent(field, descriptor, (left, right) -> Property.of(typeInformation, left, right)), // + () -> field.map(it -> Property.of(typeInformation, it)), // + () -> descriptor.map(it -> Property.of(typeInformation, it))) // + .orElseThrow(() -> new IllegalArgumentException(String.format("Couldn't find property %s on %s!", name, type))); return new SamplePersistentProperty(property, getEntity(type), typeHolder); } @@ -256,6 +271,14 @@ public class AbstractPersistentPropertyUnitTests { } + @Getter + @Setter + class GenericGetter { + T genericField; + } + + class ConcreteGetter extends GenericGetter {} + @SuppressWarnings("serial") class TestClassSet extends TreeSet {}