From 18dc75a57774fffaba84d600ccb0a24213e2b7fb Mon Sep 17 00:00:00 2001 From: Oliver Drotbohm Date: Mon, 4 Apr 2022 14:51:22 +0200 Subject: [PATCH] Fix PUT merge handling for polymorphic properties. We now replace the original value of polymorphic properties with the newly deserialized one if the type of the new one is different from the old one. We still copy all JSON ignored properties to the new instance to make sure that non-exposed, server-side state is retained. We apply the same handling for explicitly immutable source and/or target types. Fixes #2130. --- .../rest/webmvc/json/DomainObjectReader.java | 30 +++++++--- .../rest/webmvc/json/MappedProperties.java | 10 ++++ .../data/rest/webmvc/json/package-info.java | 5 +- .../json/DomainObjectReaderUnitTests.java | 59 +++++++++++++++++++ .../json/MappedPropertiesUnitTests.java | 5 ++ 5 files changed, 97 insertions(+), 12 deletions(-) diff --git a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/DomainObjectReader.java b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/DomainObjectReader.java index 5c64fb5bb..c8ba04e8f 100644 --- a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/DomainObjectReader.java +++ b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/DomainObjectReader.java @@ -42,6 +42,7 @@ import org.springframework.data.rest.webmvc.util.InputStreamHttpInputMessage; import org.springframework.data.util.ClassTypeInformation; import org.springframework.data.util.TypeInformation; import org.springframework.http.converter.HttpMessageNotReadableException; +import org.springframework.lang.Nullable; import org.springframework.util.Assert; import org.springframework.util.ObjectUtils; @@ -124,6 +125,7 @@ public class DomainObjectReader { * @param mapper must not be {@literal null}. * @return */ + @Nullable T mergeForPut(T source, T target, final ObjectMapper mapper) { Assert.notNull(mapper, "ObjectMapper must not be null!"); @@ -132,12 +134,24 @@ public class DomainObjectReader { return source; } - Class type = target.getClass(); + boolean isTypeChange = !source.getClass().isInstance(target); - return entities.getPersistentEntity(type) // - .filter(it -> !it.isImmutable()) // + boolean immutableTarget = entities.getPersistentEntity(target.getClass()) + .map(PersistentEntity::isImmutable) + .orElse(true); // Not a Spring Data managed type -> no detailed merging + + return entities.getPersistentEntity(isTypeChange ? source.getClass() : target.getClass()) // .map(it -> { + MappedProperties properties = MappedProperties.forDeserialization(it, mapper); + + if (isTypeChange || immutableTarget || it.isImmutable()) { + + copyRemainingProperties(properties.getIgnoredProperties(), target, source); + + return source; + } + MergingPropertyHandler propertyHandler = new MergingPropertyHandler(source, target, it, mapper); it.doWithProperties(propertyHandler); @@ -145,7 +159,7 @@ public class DomainObjectReader { // Need to copy unmapped properties as the PersistentProperty model currently does not contain any transient // properties - copyRemainingProperties(propertyHandler.getProperties(), source, target); + copyRemainingProperties(properties.getSpringDataUnmappedProperties(), source, target); return target; @@ -153,20 +167,20 @@ public class DomainObjectReader { } /** - * Copies the unmapped properties of the given {@link MappedProperties} from the source object to the target instance. + * Copies the given properties from the source object to the target instance. * - * @param properties must not be {@literal null}. + * @param propertyNames must not be {@literal null}. * @param source must not be {@literal null}. * @param target must not be {@literal null}. */ - private static void copyRemainingProperties(MappedProperties properties, Object source, Object target) { + private static void copyRemainingProperties(Iterable propertyNames, Object source, Object target) { PropertyAccessor sourceFieldAccessor = PropertyAccessorFactory.forDirectFieldAccess(source); PropertyAccessor sourcePropertyAccessor = PropertyAccessorFactory.forBeanPropertyAccess(source); PropertyAccessor targetFieldAccessor = PropertyAccessorFactory.forDirectFieldAccess(target); PropertyAccessor targetPropertyAccessor = PropertyAccessorFactory.forBeanPropertyAccess(target); - for (String property : properties.getSpringDataUnmappedProperties()) { + for (String property : propertyNames) { // If there's a field we can just copy it. if (targetFieldAccessor.isWritableProperty(property)) { diff --git a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/MappedProperties.java b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/MappedProperties.java index 82e1f7a9a..f8c3f582a 100644 --- a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/MappedProperties.java +++ b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/MappedProperties.java @@ -199,6 +199,16 @@ class MappedProperties { return result; } + /** + * Returns all property names of ignored properties. + * + * @return will never be {@literal null}. + * @since 3.5.11, 3.6.4 + */ + public Iterable getIgnoredProperties() { + return ignoredPropertyNames; + } + /** * Returns whether the given {@link PersistentProperty} is mapped, i.e. known to both Jackson and Spring Data. * diff --git a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/package-info.java b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/package-info.java index e863d03a8..c4d4e6bc6 100644 --- a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/package-info.java +++ b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/package-info.java @@ -13,8 +13,5 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -/** - * - * @author Oliver Gierke - */ +@org.springframework.lang.NonNullApi package org.springframework.data.rest.webmvc.json; diff --git a/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/DomainObjectReaderUnitTests.java b/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/DomainObjectReaderUnitTests.java index a266b9df7..a12cad9db 100755 --- a/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/DomainObjectReaderUnitTests.java +++ b/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/DomainObjectReaderUnitTests.java @@ -55,6 +55,9 @@ import com.fasterxml.jackson.annotation.JsonAutoDetect; import com.fasterxml.jackson.annotation.JsonAutoDetect.Visibility; import com.fasterxml.jackson.annotation.JsonIgnore; import com.fasterxml.jackson.annotation.JsonProperty; +import com.fasterxml.jackson.annotation.JsonSubTypes; +import com.fasterxml.jackson.annotation.JsonTypeInfo; +import com.fasterxml.jackson.annotation.JsonTypeInfo.As; import com.fasterxml.jackson.core.JsonParser; import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.DeserializationContext; @@ -104,6 +107,8 @@ class DomainObjectReaderUnitTests { mappingContext.getPersistentEntity(Note.class); mappingContext.getPersistentEntity(WithNullCollection.class); mappingContext.getPersistentEntity(ArrayHolder.class); + mappingContext.getPersistentEntity(Apple.class); + mappingContext.getPersistentEntity(Pear.class); mappingContext.afterPropertiesSet(); this.entities = new PersistentEntities(Collections.singleton(mappingContext)); @@ -587,6 +592,32 @@ class DomainObjectReaderUnitTests { assertThat(updated.array).containsExactly("new"); } + @Test // #2130 + void writesPolymorphicArrayWithSwitchedItemForPut() throws Exception { + + Apple apple = new Apple(); + apple.apple = "apple"; + apple.color = "red"; + apple.ignored = "ignored"; + + Pear pear = new Pear(); + pear.pear = "pear"; + + Fruit result = reader.mergeForPut(pear, apple, new ObjectMapper()); + + assertThat(result).isInstanceOfSatisfying(Pear.class, it -> { + + // Exposed property is wiped as expected for PUT + assertThat(it.color).isNull(); + + // Non-exposed state is transferred + assertThat(it.ignored).isEqualTo("ignored"); + + // Type specific state applied, too + assertThat(it.pear).isEqualTo("pear"); + }); + } + @SuppressWarnings("unchecked") private static T as(Object source, Class type) { @@ -816,4 +847,32 @@ class DomainObjectReaderUnitTests { static class ArrayHolder { String[] array; } + + // DATAREST-1026 + + @JsonAutoDetect(fieldVisibility = Visibility.ANY) + static class Basket { + + @Id Long id; + List fruits; + } + + @JsonAutoDetect(fieldVisibility = Visibility.ANY) + @JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = As.PROPERTY, property = "type") + @JsonSubTypes({ @JsonSubTypes.Type(name = "Apple", value = Apple.class), + @JsonSubTypes.Type(name = "Pear", value = Pear.class) }) + static class Fruit { + String color; + @JsonIgnore String ignored; + } + + @JsonAutoDetect(fieldVisibility = Visibility.ANY) + static class Apple extends Fruit { + String apple; + } + + @JsonAutoDetect(fieldVisibility = Visibility.ANY) + static class Pear extends Fruit { + String pear; + } } diff --git a/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/MappedPropertiesUnitTests.java b/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/MappedPropertiesUnitTests.java index 20658f5b0..06361f090 100755 --- a/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/MappedPropertiesUnitTests.java +++ b/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/MappedPropertiesUnitTests.java @@ -115,6 +115,11 @@ class MappedPropertiesUnitTests { assertThat(properties.isWritableProperty("someRandomProperty")).isTrue(); } + @Test // #2130 + void exposesIgnoredProperties() { + assertThat(properties.getIgnoredProperties()).contains("notExposedByJackson"); + } + static class Sample { public @Transient String notExposedBySpringData;