Skip shadowed properties that are not assignable to their shadowing t…

…ype.
spring-projects · Apr 19, 2021 · 0035e43 · 0035e43
1 parent acdc58c
commit 0035e43
Show file tree

Hide file tree

Showing 2 changed files with 109 additions and 28 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
@@ -17,6 +17,7 @@
 
 import java.beans.PropertyDescriptor;
 import java.lang.reflect.Field;
+import java.lang.reflect.Method;
 import java.lang.reflect.Modifier;
 import java.util.Collection;
 import java.util.Collections;
@@ -32,6 +33,7 @@
 
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
+
 import org.springframework.beans.BeanUtils;
 import org.springframework.beans.BeansException;
 import org.springframework.beans.factory.InitializingBean;
@@ -64,7 +66,6 @@
 import org.springframework.data.util.TypeInformation;
 import org.springframework.lang.Nullable;
 import org.springframework.util.Assert;
-import org.springframework.util.ClassUtils;
 import org.springframework.util.ReflectionUtils;
 import org.springframework.util.ReflectionUtils.FieldCallback;
 import org.springframework.util.ReflectionUtils.FieldFilter;
@@ -555,10 +556,9 @@ private void createAndRegisterProperty(Property input) {
 				return;
 			}
 
-			if (isKotlinOverride(property, input)) {
+			if (shouldSkipOverrideProperty(property)) {
 				return;
 			}
-
 			entity.addPersistentProperty(property);
 
 			if (property.isAssociation()) {
@@ -572,39 +572,86 @@ private void createAndRegisterProperty(Property input) {
 			property.getPersistentEntityTypes().forEach(AbstractMappingContext.this::addPersistentEntity);
 		}
 
-		private boolean isKotlinOverride(P property, Property input) {
+		protected boolean shouldSkipOverrideProperty(P property) {
 
-			if (!KotlinDetector.isKotlinPresent() || !input.getField().isPresent()) {
-				return false;
-			}
+			P existingProperty = entity.getPersistentProperty(property.getName());
 
-			Field field = input.getField().get();
-			if (!KotlinDetector.isKotlinType(field.getDeclaringClass())) {
+			if (existingProperty == null) {
 				return false;
 			}
 
-			for (P existingProperty : entity) {
+			Class<?> declaringClass = getDeclaringClass(property);
+			Class<?> existingDeclaringClass = getDeclaringClass(existingProperty);
 
-				if (!property.getName().equals(existingProperty.getName())) {
-					continue;
-				}
+			Class<?> propertyType = getPropertyType(property);
+			Class<?> existingPropertyType = getPropertyType(existingProperty);
 
-				if (field.getDeclaringClass() != entity.getType()
-						&& ClassUtils.isAssignable(field.getDeclaringClass(), entity.getType())) {
+			if (!existingPropertyType.isAssignableFrom(propertyType)) {
 
-					if (LOGGER.isTraceEnabled()) {
-						LOGGER.trace(String.format("Skipping '%s.%s' property declaration shadowed by '%s %s' in '%s'. ",
-								field.getDeclaringClass().getName(), property.getName(), property.getType().getSimpleName(),
-								property.getName(), entity.getType().getSimpleName()));
-					}
-					return true;
+				if (LOGGER.isDebugEnabled()) {
+					LOGGER.warn(String.format("Offending property declaration in '%s %s.%s' shadowing '%s %s.%s' in '%s'. ",
+							propertyType.getSimpleName(), declaringClass.getName(), property.getName(),
+							existingPropertyType.getSimpleName(), existingDeclaringClass.getName(), existingProperty.getName(),
+							entity.getType().getSimpleName()));
 				}
+
+				return true;
 			}
 
 			return false;
 		}
+
+		private Class<?> getDeclaringClass(PersistentProperty<?> persistentProperty) {
+
+			Field field = persistentProperty.getField();
+			if (field != null) {
+				return field.getDeclaringClass();
+			}
+
+			Method accessor = persistentProperty.getGetter();
+
+			if (accessor == null) {
+				accessor = persistentProperty.getSetter();
+			}
+
+			if (accessor == null) {
+				accessor = persistentProperty.getWither();
+			}
+
+			if (accessor != null) {
+				return accessor.getDeclaringClass();
+			}
+
+			return persistentProperty.getOwner().getType();
+		}
+
+		private Class<?> getPropertyType(PersistentProperty<?> persistentProperty) {
+
+			Field field = persistentProperty.getField();
+			if (field != null) {
+				return field.getType();
+			}
+
+			Method getter = persistentProperty.getGetter();
+			if (getter != null) {
+				return getter.getReturnType();
+			}
+
+			Method setter = persistentProperty.getSetter();
+			if (setter != null) {
+				return setter.getParameterTypes()[0];
+			}
+
+			Method wither = persistentProperty.getWither();
+			if (wither != null) {
+				return wither.getParameterTypes()[0];
+			}
+
+			return persistentProperty.getType();
+		}
 	}
 
+
 	/**
 	 * Filter rejecting static fields as well as artificially introduced ones. See
 	 * {@link PersistentPropertyFilter#UNMAPPED_PROPERTIES} for details.

diff --git a/src/test/java/org/springframework/data/mapping/context/AbstractMappingContextUnitTests.java b/src/test/java/org/springframework/data/mapping/context/AbstractMappingContextUnitTests.java
@@ -62,7 +62,7 @@
  */
 class AbstractMappingContextUnitTests {
 
-	SampleMappingContext context;
+	private SampleMappingContext context;
 
 	@BeforeEach
 	void setUp() {
@@ -224,27 +224,28 @@ void cleansUpCacheForRuntimeException() {
 	}
 
 	@Test // DATACMNS-1509
-	public void shouldIgnoreKotlinOverrideCtorPropertyInSuperClass() {
+	void shouldIgnoreKotlinOverrideCtorPropertyInSuperClass() {
 
 		BasicPersistentEntity<Object, SamplePersistentProperty> entity = context
 				.getPersistentEntity(ClassTypeInformation.from(ShadowingPropertyTypeWithCtor.class));
 		entity.doWithProperties((PropertyHandler<SamplePersistentProperty>) property -> {
-			assertThat(property.getField().getDeclaringClass()).isNotEqualTo(ShadowedPropertyTypeWithCtor.class);
+			assertThat(property.getField().getDeclaringClass()).isIn(ShadowingPropertyTypeWithCtor.class,
+					ShadowedPropertyTypeWithCtor.class);
 		});
 	}
 
 	@Test // DATACMNS-1509
-	public void shouldIgnoreKotlinOverridePropertyInSuperClass() {
+	void shouldIncludeAssignableKotlinOverridePropertyInSuperClass() {
 
 		BasicPersistentEntity<Object, SamplePersistentProperty> entity = context
 				.getPersistentEntity(ClassTypeInformation.from(ShadowingPropertyType.class));
 		entity.doWithProperties((PropertyHandler<SamplePersistentProperty>) property -> {
-			assertThat(property.getField().getDeclaringClass()).isNotEqualTo(ShadowedPropertyType.class);
+			assertThat(property.getField().getDeclaringClass()).isIn(ShadowedPropertyType.class, ShadowingPropertyType.class);
 		});
 	}
 
 	@Test // DATACMNS-1509
-	public void shouldStillIncludeNonKotlinShadowedPropertyInSuperClass() {
+	void shouldIncludeAssignableShadowedPropertyInSuperClass() {
 
 		BasicPersistentEntity<Object, SamplePersistentProperty> entity = context
 				.getPersistentEntity(ClassTypeInformation.from(ShadowingProperty.class));
@@ -254,6 +255,16 @@ public void shouldStillIncludeNonKotlinShadowedPropertyInSuperClass() {
 		).isNotEmpty();
 	}
 
+	@Test // DATACMNS-1509
+	void shouldIgnoreOverridePropertyInSuperClass() {
+
+		BasicPersistentEntity<Object, SamplePersistentProperty> entity = context
+				.getPersistentEntity(ClassTypeInformation.from(ShadowingPropertyNotAssignable.class));
+		entity.doWithProperties((PropertyHandler<SamplePersistentProperty>) property -> {
+			assertThat(property.getField().getDeclaringClass()).isEqualTo(ShadowingPropertyNotAssignable.class);
+		});
+	}
+
 	private static void assertHasEntityFor(Class<?> type, SampleMappingContext context, boolean expected) {
 
 		boolean found = false;
@@ -275,7 +286,7 @@ class Person {
 		LocalDateTime date;
 	}
 
-	class Unsupported {
+	private class Unsupported {
 
 	}
 
@@ -377,4 +388,27 @@ public String getValue() {
 		}
 	}
 
+	static class ShadowedPropertyNotAssignable {
+
+		private String value;
+
+	}
+
+	static class ShadowingPropertyNotAssignable extends ShadowedPropertyNotAssignable {
+
+		private int value;
+
+		ShadowingPropertyNotAssignable(int value) {
+			this.value = value;
+		}
+
+		public void setValue(int value) {
+			this.value = value;
+		}
+
+		public int getValue() {
+			return value;
+		}
+	}
+
 }