DATACMNS-1509 - Ignore override properties with non-assignable types when creating PersistentEntity.

pom.xml

@@ -5,7 +5,7 @@
 
 	<groupId>org.springframework.data</groupId>
 	<artifactId>spring-data-commons</artifactId>
-	<version>2.6.0-SNAPSHOT</version>
+	<version>2.6.0.DATACMNS-1509-SNAPSHOT</version>
 
 	<name>Spring Data Core</name>
 

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
@@ -146,7 +146,7 @@ For the domain class to be eligible for such optimization, it needs to adhere to
 - Types must not reside in the default or under the `java` package.
 - Types and their constructors must be `public`
 - Types that are inner classes must be `static`.
-- The used Java Runtime must allow for declaring classes in the originating `ClassLoader`. Java 9 and newer impose certain limitations.
+- The used Java Runtime must allow for declaring classes in the originating `ClassLoader`.Java 9 and newer impose certain limitations.
 
 By default, Spring Data attempts to use generated property accessors and falls back to reflection-based ones if a limitation is detected.
 ****
@@ -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 automatically 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 @@
 
 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;
@@ -30,6 +31,9 @@
 import java.util.function.Predicate;
 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;
@@ -87,6 +91,8 @@
 public abstract class AbstractMappingContext<E extends MutablePersistentEntity<?, P>, P extends PersistentProperty<P>>
 		implements MappingContext<E, P>, ApplicationEventPublisherAware, ApplicationContextAware, InitializingBean {
 
+	private static final Logger LOGGER = LoggerFactory.getLogger(MappingContext.class);
+
 	private final Optional<E> NONE = Optional.empty();
 	private final Map<TypeInformation<?>, Optional<E>> persistentEntities = new HashMap<>();
 	private final PersistentPropertyAccessorFactory persistentPropertyAccessorFactory;
@@ -550,6 +556,9 @@ private void createAndRegisterProperty(Property input) {
 				return;
 			}
 
+			if (shouldSkipOverrideProperty(property)) {
+				return;
+			}
 			entity.addPersistentProperty(property);
 
 			if (property.isAssociation()) {
@@ -562,8 +571,87 @@ private void createAndRegisterProperty(Property input) {
 
 			property.getPersistentEntityTypes().forEach(AbstractMappingContext.this::addPersistentEntity);
 		}
+
+		protected boolean shouldSkipOverrideProperty(P property) {
+
+			P existingProperty = entity.getPersistentProperty(property.getName());
+
+			if (existingProperty == null) {
+				return false;
+			}
+
+			Class<?> declaringClass = getDeclaringClass(property);
+			Class<?> existingDeclaringClass = getDeclaringClass(existingProperty);
+
+			Class<?> propertyType = getPropertyType(property);
+			Class<?> existingPropertyType = getPropertyType(existingProperty);
+
+			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()));
+				}
+
+				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/main/java/org/springframework/data/mapping/model/BeanWrapper.java

@@ -25,10 +25,10 @@
 import java.util.List;
 import java.util.Map;
 
-import org.springframework.core.KotlinDetector;
 import org.springframework.data.mapping.MappingException;
 import org.springframework.data.mapping.PersistentProperty;
 import org.springframework.data.mapping.PersistentPropertyAccessor;
+import org.springframework.data.util.KotlinReflectionUtils;
 import org.springframework.lang.Nullable;
 import org.springframework.util.Assert;
 import org.springframework.util.ConcurrentReferenceHashMap;
@@ -77,7 +77,7 @@ public void setProperty(PersistentProperty<?> property, @Nullable Object value)
 					return;
 				}
 
-				if (KotlinDetector.isKotlinType(property.getOwner().getType())) {
+				if (KotlinReflectionUtils.isDataClass(property.getOwner().getType())) {
 
 					this.bean = (T) KotlinCopyUtil.setProperty(property, bean, value);
 					return;

src/main/java/org/springframework/data/util/KotlinReflectionUtils.java

@@ -66,6 +66,22 @@ public static boolean isSupportedKotlinClass(Class<?> type) {
 				.anyMatch(it -> Integer.valueOf(KotlinClassHeaderKind.CLASS.id).equals(it));
 	}
 
+	/**
+	 * Return {@literal true} if the specified class is a Kotlin data class.
+	 *
+	 * @return {@literal true} if {@code type} is a Kotlin data class.
+	 * @since 2.6
+	 */
+	public static boolean isDataClass(Class<?> type) {
+
+		if (!KotlinDetector.isKotlinType(type)) {
+			return false;
+		}
+
+		KClass<?> kotlinClass = JvmClassMappingKt.getKotlinClass(type);
+		return kotlinClass.isData();
+	}
+
 	/**
 	 * Returns a {@link KFunction} instance corresponding to the given Java {@link Method} instance, or {@code null} if
 	 * this method cannot be represented by a Kotlin function.

src/test/java/org/springframework/data/mapping/KotlinModelTypes.kt

@@ -0,0 +1,30 @@
+/*
+ * Copyright 2019 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.
+ * You may obtain a copy of the License at
+ *
+ *      https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.springframework.data.mapping
+
+open class ShadowedPropertyType {
+	open val shadowedProperty: Int = 1
+}
+
+class ShadowingPropertyType : ShadowedPropertyType() {
+	override var shadowedProperty: Int = 10
+}
+
+open class ShadowedPropertyTypeWithCtor(open val shadowedProperty: Int)
+
+class ShadowingPropertyTypeWithCtor(val someValue: String, override var shadowedProperty: Int = 1) : ShadowedPropertyTypeWithCtor(shadowedProperty)
+
+