From adcd7e74efb696b34b7e516556cb6282bf66befd 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 84a5c45b5..3ac6c3f21 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.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 T readPut(final ObjectNode source, T target, final ObjectMapper mappe * @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 @@ T mergeForPut(T source, T target, final ObjectMapper mapper) { 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 @@ T mergeForPut(T source, T target, final ObjectMapper mapper) { // 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 @@ T mergeForPut(T source, T target, final ObjectMapper mapper) { } /** - * 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 @@ public Iterable getSpringDataUnmappedProperties() { 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 2215526be..668098226 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.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 @@ void setUp() { 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 @@ void arraysCanBeResizedDuringMerge() throws Exception { 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) { @@ -812,4 +843,32 @@ static class WithNullCollection { 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 @@ void exposesExistanceOfCatchAllMethod() { assertThat(properties.isWritableProperty("someRandomProperty")).isTrue(); } + @Test // #2130 + void exposesIgnoredProperties() { + assertThat(properties.getIgnoredProperties()).contains("notExposedByJackson"); + } + static class Sample { public @Transient String notExposedBySpringData;