Polishing.

Skip shadowed properties that are not assignable to their shadowing type.

Extend tests. Document property overrides.

Original pull request: #390.
See #1911.

src/main/asciidoc/object-mapping.adoc

@@ -76,8 +76,8 @@ For that we use the following algorithm:
 1. If the property is immutable but exposes a `with…` method (see below), we use the `with…` method to create a new entity instance with the new property value.
 2. If property access (i.e. access through getters and setters) is defined, we're invoking the setter method.
 3. If the property is mutable we set the field directly.
-3. If the property is immutable we're using the constructor to be used by persistence operations (see <<mapping.object-creation>>) to create a copy of the instance.
-4. By default, we set the field value directly.
+4. If the property is immutable we're using the constructor to be used by persistence operations (see <<mapping.object-creation>>) to create a copy of the instance.
+5. By default, we set the field value directly.
 
 [[mapping.property-population.details]]
 .Property population internals
@@ -221,6 +221,73 @@ It's an established pattern to rather use static factory methods to expose these
 * _For identifiers to be generated, still use a final field in combination with an all-arguments persistence constructor (preferred) or a `with…` method_ --
 * _Use Lombok to avoid boilerplate code_ -- As persistence operations usually require a constructor taking all arguments, their declaration becomes a tedious repetition of boilerplate parameter to field assignments that can best be avoided by using Lombok's `@AllArgsConstructor`.
 
+[[mapping.general-recommendations.override.properties]]
+=== Overriding Properties
+
+Java's allows a flexible design of domain classes where a subclass could define a property that is already declared with the same name in its superclass.
+Consider the following example:
+
+====
+[source,java]
+----
+public class SuperType {
+
+   private CharSequence field;
+
+   public SuperType(CharSequence field) {
+      this.field = field;
+   }
+
+   public CharSequence getField() {
+      return this.field;
+   }
+
+   public void setField(CharSequence field) {
+      this.field = field;
+   }
+}
+
+public class SubType extends SuperType {
+
+   private String field;
+
+   public SubType(String field) {
+      super(field);
+      this.field = field;
+   }
+
+   @Override
+   public String getField() {
+      return this.field;
+   }
+
+   public void setField(String field) {
+      this.field = field;
+
+      // optional
+      super.setField(field);
+   }
+}
+----
+====
+
+Both classes define a `field` using assignable types. `SubType` however shadows `SuperType.field`.
+Depending on the class design, using the constructor could be the only default approach to set `SuperType.field`.
+Alternatively, calling `super.setField(…)` in the setter could set the `field` in `SuperType`.
+All these mechanisms create conflicts to some degree because the properties share the same name yet might represent two distinct values.
+Spring Data skips super-type properties if types are not assignable.
+That is, the type of the overridden property must be assignable to its super-type property type to be registered as override, otherwise the super-type property is considered transient.
+We generally recommend using distinct property names.
+
+Spring Data modules generally support overridden properties holding different values.
+From a programming model perspective there are a few things to consider:
+
+1. Which property should be persisted (default to all declared properties)?
+You can exclude properties by annotating these with `@Transient`.
+2. How to represent properties in your data store?
+Using the same field/column name for different values typically leads to corrupt data so you should annotate least one of the properties using an explicit field/column name.
+3. Using `@AccessType(PROPERTY)` cannot be used as the super-property cannot be generally set without making any further assumptions of the setter implementation.
+
 [[mapping.kotlin]]
 == Kotlin support
 
@@ -277,3 +344,78 @@ data class Person(val id: String, val name: String)
 
 This class is effectively immutable.
 It allows creating new instances as Kotlin generates a `copy(…)` method that creates new object instances copying all property values from the existing object and applying property values provided as arguments to the method.
+
+[[mapping.kotlin.override.properties]]
+=== Kotlin Overriding Properties
+
+Kotlin allows declaring https://kotlinlang.org/docs/inheritance.html#overriding-properties[property overrides] to alter properties in subclasses.
+
+====
+[source,kotlin]
+----
+open class SuperType(open var field: Int)
+
+class SubType(override var field: Int = 1) :
+	SuperType(field) {
+}
+----
+====
+
+Such an arrangement renders two properties with the name `field`.
+Kotlin generates property accessors (getters and setters) for each property in each class.
+Effectively, the code looks like as follows:
+
+====
+[source,java]
+----
+public class SuperType {
+
+   private int field;
+
+   public SuperType(int field) {
+      this.field = field;
+   }
+
+   public int getField() {
+      return this.field;
+   }
+
+   public void setField(int field) {
+      this.field = field;
+   }
+}
+
+public final class SubType extends SuperType {
+
+   private int field;
+
+   public SubType(int field) {
+      super(field);
+      this.field = field;
+   }
+
+   public int getField() {
+      return this.field;
+   }
+
+   public void setField(int field) {
+      this.field = field;
+   }
+}
+----
+====
+
+Getters and setters on `SubType` set only `SubType.field` and not `SuperType.field`.
+In such an arrangement, using the constructor is the only default approach to set `SuperType.field`.
+Adding a method to `SubType` to set `SuperType.field` via `this.SuperType.field = …` is possible but falls outside of supported conventions.
+Property overrides create conflicts to some degree because the properties share the same name yet might represent two distinct values.
+We generally recommend using distinct property names.
+
+Spring Data modules generally support overridden properties holding different values.
+From a programming model perspective there are a few things to consider:
+
+1. Which property should be persisted (default to all declared properties)?
+You can exclude properties by annotating these with `@Transient`.
+2. How to represent properties in your data store?
+Using the same field/column name for different values typically leads to corrupt data so you should annotate least one of the properties using an explicit field/column name.
+3. Using `@AccessType(PROPERTY)` cannot be used as the super-property cannot be set.

src/main/java/org/springframework/data/mapping/context/AbstractMappingContext.java

@@ -17,6 +17,7 @@ package org.springframework.data.mapping.context;
 
 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 java.util.stream.Collectors;
 
 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.Streamable;
 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 @@ public abstract class AbstractMappingContext<E extends MutablePersistentEntity<?
 				return;
 			}
 
-			if (isKotlinOverride(property, input)) {
+			if (shouldSkipOverrideProperty(property)) {
 				return;
 			}
-
 			entity.addPersistentProperty(property);
 
 			if (property.isAssociation()) {
@@ -572,39 +572,86 @@ public abstract class AbstractMappingContext<E extends MutablePersistentEntity<?
 			property.getPersistentEntityTypes().forEach(AbstractMappingContext.this::addPersistentEntity);
 		}
 
-		private boolean isKotlinOverride(P property, Property input) {
+		protected boolean shouldSkipOverrideProperty(P property) {
 
-			if (!KotlinDetector.isKotlinPresent() || !input.getField().isPresent()) {
+			P existingProperty = entity.getPersistentProperty(property.getName());
+
+			if (existingProperty == null) {
 				return false;
 			}
 
-			Field field = input.getField().get();
-			if (!KotlinDetector.isKotlinType(field.getDeclaringClass())) {
-				return false;
-			}
+			Class<?> declaringClass = getDeclaringClass(property);
+			Class<?> existingDeclaringClass = getDeclaringClass(existingProperty);
 
-			for (P existingProperty : entity) {
+			Class<?> propertyType = getPropertyType(property);
+			Class<?> existingPropertyType = getPropertyType(existingProperty);
 
-				if (!property.getName().equals(existingProperty.getName())) {
-					continue;
+			if (!propertyType.isAssignableFrom(existingPropertyType)) {
+
+				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()));
 				}
 
-				if (field.getDeclaringClass() != entity.getType()
-						&& ClassUtils.isAssignable(field.getDeclaringClass(), entity.getType())) {
-
-					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;
-				}
+				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.

src/test/java/org/springframework/data/mapping/context/AbstractMappingContextUnitTests.java

@@ -62,7 +62,7 @@ import org.springframework.data.util.TypeInformation;
  */
 class AbstractMappingContextUnitTests {
 
-	SampleMappingContext context;
+	private SampleMappingContext context;
 
 	@BeforeEach
 	void setUp() {
@@ -223,35 +223,53 @@ class AbstractMappingContextUnitTests {
 				.isThrownBy(() -> context.getPersistentEntity(Unsupported.class));
 	}
 
-	@Test // DATACMNS-1509
-	public void shouldIgnoreKotlinOverrideCtorPropertyInSuperClass() {
+	@Test // GH-3113
+	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() {
+	@Test // GH-3113
+	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() {
+	@Test // GH-3113
+	void shouldIncludeAssignableShadowedPropertyInSuperClass() {
 
 		BasicPersistentEntity<Object, SamplePersistentProperty> entity = context
-				.getPersistentEntity(ClassTypeInformation.from(ShadowingProperty.class));
+				.getPersistentEntity(ClassTypeInformation.from(ShadowingPropertyAssignable.class));
 
 		assertThat(StreamUtils.createStreamFromIterator(entity.iterator())
-				.filter(it -> it.getField().getDeclaringClass().equals(ShadowedProperty.class)).findFirst() //
+				.filter(it -> it.getField().getDeclaringClass().equals(ShadowedPropertyAssignable.class)).findFirst() //
 		).isNotEmpty();
+
+		assertThat(entity).hasSize(2);
+
+		entity.doWithProperties((PropertyHandler<SamplePersistentProperty>) property -> {
+			assertThat(property.getField().getDeclaringClass()).isIn(ShadowedPropertyAssignable.class,
+					ShadowingPropertyAssignable.class);
+		});
+	}
+
+	@Test // GH-3113
+	void shouldIgnoreNonAssignableOverridePropertyInSuperClass() {
+
+		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) {
@@ -275,7 +293,7 @@ class AbstractMappingContextUnitTests {
 		LocalDateTime date;
 	}
 
-	class Unsupported {
+	private class Unsupported {
 
 	}
 
@@ -377,4 +395,41 @@ class AbstractMappingContextUnitTests {
 		}
 	}
 
+	static class ShadowedPropertyNotAssignable {
+
+		private String value;
+	}
+
+	static class ShadowingPropertyNotAssignable extends ShadowedPropertyNotAssignable {
+
+		private Integer value;
+
+		ShadowingPropertyNotAssignable(Integer value) {
+			this.value = value;
+		}
+
+		public Integer getValue() {
+			return value;
+		}
+
+		public void setValue(Integer value) {
+			this.value = value;
+		}
+	}
+
+	static class ShadowedPropertyAssignable {
+
+		private Object value;
+
+	}
+
+	static class ShadowingPropertyAssignable extends ShadowedPropertyAssignable {
+
+		private Integer value;
+
+		ShadowingPropertyAssignable(Integer value) {
+			this.value = value;
+		}
+	}
+
 }