From 3ab0b7d73863edd9732b4009221042fd55ff30cc Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Fri, 5 Apr 2019 18:33:11 +0200 Subject: [PATCH 1/5] DATACMNS-1509 - Prepare issue branch. --- pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pom.xml b/pom.xml index fd73ef7f43..5a3c4d2c06 100644 --- a/pom.xml +++ b/pom.xml @@ -5,7 +5,7 @@ org.springframework.data spring-data-commons - 2.6.0-SNAPSHOT + 2.6.0.DATACMNS-1509-SNAPSHOT Spring Data Core From 74ae72e3186f3df97cc4c5bc40b3e9ad3540c4ce Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Fri, 5 Apr 2019 18:34:40 +0200 Subject: [PATCH 2/5] DATACMNS-1509 - Ignore Kotlin override properties when creating PersistentEntity. --- .../context/AbstractMappingContext.java | 41 +++++++++++ .../data/mapping/KotlinModelTypes.kt | 30 ++++++++ .../AbstractMappingContextUnitTests.java | 71 +++++++++++++++++++ 3 files changed, 142 insertions(+) create mode 100644 src/test/java/org/springframework/data/mapping/KotlinModelTypes.kt 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 f04bdd5ac3..02a0902f87 100644 --- a/src/main/java/org/springframework/data/mapping/context/AbstractMappingContext.java +++ b/src/main/java/org/springframework/data/mapping/context/AbstractMappingContext.java @@ -30,6 +30,8 @@ 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; @@ -62,6 +64,7 @@ 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; @@ -87,6 +90,8 @@ public abstract class AbstractMappingContext, P extends PersistentProperty

> implements MappingContext, ApplicationEventPublisherAware, ApplicationContextAware, InitializingBean { + private static final Logger LOGGER = LoggerFactory.getLogger(MappingContext.class); + private final Optional NONE = Optional.empty(); private final Map, Optional> persistentEntities = new HashMap<>(); private final PersistentPropertyAccessorFactory persistentPropertyAccessorFactory; @@ -550,6 +555,10 @@ private void createAndRegisterProperty(Property input) { return; } + if (isKotlinOverride(property, input)) { + return; + } + entity.addPersistentProperty(property); if (property.isAssociation()) { @@ -562,6 +571,38 @@ private void createAndRegisterProperty(Property input) { property.getPersistentEntityTypes().forEach(AbstractMappingContext.this::addPersistentEntity); } + + private boolean isKotlinOverride(P property, Property input) { + + if (!KotlinDetector.isKotlinPresent() || !input.getField().isPresent()) { + return false; + } + + Field field = input.getField().get(); + if (!KotlinDetector.isKotlinType(field.getDeclaringClass())) { + return false; + } + + for (P existingProperty : entity) { + + if (!property.getName().equals(existingProperty.getName())) { + continue; + } + + 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 false; + } } /** diff --git a/src/test/java/org/springframework/data/mapping/KotlinModelTypes.kt b/src/test/java/org/springframework/data/mapping/KotlinModelTypes.kt new file mode 100644 index 0000000000..23ddc2f026 --- /dev/null +++ b/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) + + diff --git a/src/test/java/org/springframework/data/mapping/context/AbstractMappingContextUnitTests.java b/src/test/java/org/springframework/data/mapping/context/AbstractMappingContextUnitTests.java index d49ed04f7d..4b780a6d19 100755 --- a/src/test/java/org/springframework/data/mapping/context/AbstractMappingContextUnitTests.java +++ b/src/test/java/org/springframework/data/mapping/context/AbstractMappingContextUnitTests.java @@ -41,9 +41,15 @@ import org.springframework.data.annotation.Id; import org.springframework.data.mapping.MappingException; import org.springframework.data.mapping.PersistentEntity; +import org.springframework.data.mapping.PropertyHandler; +import org.springframework.data.mapping.ShadowedPropertyType; +import org.springframework.data.mapping.ShadowedPropertyTypeWithCtor; +import org.springframework.data.mapping.ShadowingPropertyType; +import org.springframework.data.mapping.ShadowingPropertyTypeWithCtor; import org.springframework.data.mapping.model.BasicPersistentEntity; import org.springframework.data.mapping.model.SimpleTypeHolder; import org.springframework.data.util.ClassTypeInformation; +import org.springframework.data.util.StreamUtils; import org.springframework.data.util.TypeInformation; /** @@ -52,6 +58,7 @@ * @author Oliver Gierke * @author Thomas Darimont * @author Mark Paluch + * @author Christoph Stobl */ class AbstractMappingContextUnitTests { @@ -216,6 +223,37 @@ void cleansUpCacheForRuntimeException() { .isThrownBy(() -> context.getPersistentEntity(Unsupported.class)); } + @Test // DATACMNS-1509 + public void shouldIgnoreKotlinOverrideCtorPropertyInSuperClass() { + + BasicPersistentEntity entity = context + .getPersistentEntity(ClassTypeInformation.from(ShadowingPropertyTypeWithCtor.class)); + entity.doWithProperties((PropertyHandler) property -> { + assertThat(property.getField().getDeclaringClass()).isNotEqualTo(ShadowedPropertyTypeWithCtor.class); + }); + } + + @Test // DATACMNS-1509 + public void shouldIgnoreKotlinOverridePropertyInSuperClass() { + + BasicPersistentEntity entity = context + .getPersistentEntity(ClassTypeInformation.from(ShadowingPropertyType.class)); + entity.doWithProperties((PropertyHandler) property -> { + assertThat(property.getField().getDeclaringClass()).isNotEqualTo(ShadowedPropertyType.class); + }); + } + + @Test // DATACMNS-1509 + public void shouldStillIncludeNonKotlinShadowedPropertyInSuperClass() { + + BasicPersistentEntity entity = context + .getPersistentEntity(ClassTypeInformation.from(ShadowingProperty.class)); + + assertThat(StreamUtils.createStreamFromIterator(entity.iterator()) + .filter(it -> it.getField().getDeclaringClass().equals(ShadowedProperty.class)).findFirst() // + ).isNotEmpty(); + } + private static void assertHasEntityFor(Class type, SampleMappingContext context, boolean expected) { boolean found = false; @@ -306,4 +344,37 @@ public void verify() { }; } } + + static class ShadowedProperty { + + private final String value; + + ShadowedProperty(String value) { + this.value = value; + } + + public String getValue() { + return value; + } + } + + static class ShadowingProperty extends ShadowedProperty { + + private String value; + + ShadowingProperty(String value) { + super(value); + this.value = value; + } + + public void setValue(String value) { + this.value = value; + } + + @Override + public String getValue() { + return value; + } + } + } From acdc58c987a7fb75f619fefd429b52c1f5834168 Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Mon, 19 Apr 2021 10:29:55 +0200 Subject: [PATCH 3/5] Limit BeanWrapper's KotlinCopyUtil to Kotlin Data classes. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, we tried to invoke the copy(…) method on all Kotlin classes whereas the copy(…) method is only specific to Kotlin data classes. --- .../data/mapping/model/BeanWrapper.java | 4 ++-- .../data/util/KotlinReflectionUtils.java | 16 ++++++++++++++++ 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/springframework/data/mapping/model/BeanWrapper.java b/src/main/java/org/springframework/data/mapping/model/BeanWrapper.java index 344063e7c1..5f49c81e79 100644 --- a/src/main/java/org/springframework/data/mapping/model/BeanWrapper.java +++ b/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; diff --git a/src/main/java/org/springframework/data/util/KotlinReflectionUtils.java b/src/main/java/org/springframework/data/util/KotlinReflectionUtils.java index 5bc0b2a6ae..0c730d9f8a 100644 --- a/src/main/java/org/springframework/data/util/KotlinReflectionUtils.java +++ b/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. From 0035e436ded9040dde38aa4cb87dfe27620222cb Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Mon, 19 Apr 2021 10:30:29 +0200 Subject: [PATCH 4/5] Skip shadowed properties that are not assignable to their shadowing type. --- .../context/AbstractMappingContext.java | 89 ++++++++++++++----- .../AbstractMappingContextUnitTests.java | 48 ++++++++-- 2 files changed, 109 insertions(+), 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 index 02a0902f87..aa12ec140a 100644 --- 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 index 4b780a6d19..8993aaa42c 100755 --- 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 entity = context .getPersistentEntity(ClassTypeInformation.from(ShadowingPropertyTypeWithCtor.class)); entity.doWithProperties((PropertyHandler) 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 entity = context .getPersistentEntity(ClassTypeInformation.from(ShadowingPropertyType.class)); entity.doWithProperties((PropertyHandler) 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 entity = context .getPersistentEntity(ClassTypeInformation.from(ShadowingProperty.class)); @@ -254,6 +255,16 @@ public void shouldStillIncludeNonKotlinShadowedPropertyInSuperClass() { ).isNotEmpty(); } + @Test // DATACMNS-1509 + void shouldIgnoreOverridePropertyInSuperClass() { + + BasicPersistentEntity entity = context + .getPersistentEntity(ClassTypeInformation.from(ShadowingPropertyNotAssignable.class)); + entity.doWithProperties((PropertyHandler) 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; + } + } + } From 4a79e2650e5ccb8257d002f78e68ab85e2515232 Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Tue, 20 Apr 2021 16:46:09 +0200 Subject: [PATCH 5/5] Polishing. Extend tests. Document property overrides. --- src/main/asciidoc/object-mapping.adoc | 148 +++++++++++++++++- .../context/AbstractMappingContext.java | 2 +- .../AbstractMappingContextUnitTests.java | 47 ++++-- 3 files changed, 180 insertions(+), 17 deletions(-) diff --git a/src/main/asciidoc/object-mapping.adoc b/src/main/asciidoc/object-mapping.adoc index 1adecf696a..1d3760d239 100644 --- a/src/main/asciidoc/object-mapping.adoc +++ b/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 <>) 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 <>) 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. 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 aa12ec140a..63f01a3552 100644 --- a/src/main/java/org/springframework/data/mapping/context/AbstractMappingContext.java +++ b/src/main/java/org/springframework/data/mapping/context/AbstractMappingContext.java @@ -586,7 +586,7 @@ protected boolean shouldSkipOverrideProperty(P property) { Class propertyType = getPropertyType(property); Class existingPropertyType = getPropertyType(existingProperty); - if (!existingPropertyType.isAssignableFrom(propertyType)) { + if (!propertyType.isAssignableFrom(existingPropertyType)) { if (LOGGER.isDebugEnabled()) { LOGGER.warn(String.format("Offending property declaration in '%s %s.%s' shadowing '%s %s.%s' in '%s'. ", diff --git a/src/test/java/org/springframework/data/mapping/context/AbstractMappingContextUnitTests.java b/src/test/java/org/springframework/data/mapping/context/AbstractMappingContextUnitTests.java index 8993aaa42c..650afde73c 100755 --- a/src/test/java/org/springframework/data/mapping/context/AbstractMappingContextUnitTests.java +++ b/src/test/java/org/springframework/data/mapping/context/AbstractMappingContextUnitTests.java @@ -223,7 +223,7 @@ void cleansUpCacheForRuntimeException() { .isThrownBy(() -> context.getPersistentEntity(Unsupported.class)); } - @Test // DATACMNS-1509 + @Test // GH-3113 void shouldIgnoreKotlinOverrideCtorPropertyInSuperClass() { BasicPersistentEntity entity = context @@ -234,7 +234,7 @@ void shouldIgnoreKotlinOverrideCtorPropertyInSuperClass() { }); } - @Test // DATACMNS-1509 + @Test // GH-3113 void shouldIncludeAssignableKotlinOverridePropertyInSuperClass() { BasicPersistentEntity entity = context @@ -244,19 +244,26 @@ void shouldIncludeAssignableKotlinOverridePropertyInSuperClass() { }); } - @Test // DATACMNS-1509 + @Test // GH-3113 void shouldIncludeAssignableShadowedPropertyInSuperClass() { BasicPersistentEntity 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) property -> { + assertThat(property.getField().getDeclaringClass()).isIn(ShadowedPropertyAssignable.class, + ShadowingPropertyAssignable.class); + }); } - @Test // DATACMNS-1509 - void shouldIgnoreOverridePropertyInSuperClass() { + @Test // GH-3113 + void shouldIgnoreNonAssignableOverridePropertyInSuperClass() { BasicPersistentEntity entity = context .getPersistentEntity(ClassTypeInformation.from(ShadowingPropertyNotAssignable.class)); @@ -391,23 +398,37 @@ public String getValue() { static class ShadowedPropertyNotAssignable { private String value; - } static class ShadowingPropertyNotAssignable extends ShadowedPropertyNotAssignable { - private int value; + private Integer value; - ShadowingPropertyNotAssignable(int value) { + ShadowingPropertyNotAssignable(Integer value) { this.value = value; } - public void setValue(int value) { + public Integer getValue() { + return value; + } + + public void setValue(Integer value) { this.value = value; } + } - public int getValue() { - return value; + static class ShadowedPropertyAssignable { + + private Object value; + + } + + static class ShadowingPropertyAssignable extends ShadowedPropertyAssignable { + + private Integer value; + + ShadowingPropertyAssignable(Integer value) { + this.value = value; } }