From 1fadf164ae305bb6227945143846a2196c95cc9b Mon Sep 17 00:00:00 2001 From: Oliver Drotbohm Date: Wed, 30 Aug 2023 21:26:40 +0200 Subject: [PATCH] Reinstate new object addition for collections in patch operations. The removal of manual collection value appendance for primitives in the context of GH-2261 lead to complex object to be appended onto a collection not being deserialized at all. This deserialization is now reinstantiated with the value to be added looked up by reading the parent node into the collections element type using a JSON Pointer expression of /$propertyName/$index. This is done to make sure that @JsonDeserialize annotations on the property kick in for the deserialization of the individual elements. We now also shortcut the entire merge algorithm for collections that are empty, contain primitives or enums. Fixes GH-2287. --- .../rest/webmvc/json/DomainObjectReader.java | 51 ++++++++++++++++--- .../json/DomainObjectReaderUnitTests.java | 31 +++++++++++ 2 files changed, 76 insertions(+), 6 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 593e1a50c..0c98c7d2e 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 @@ -15,6 +15,7 @@ */ package org.springframework.data.rest.webmvc.json; +import java.io.IOException; import java.io.InputStream; import java.util.ArrayList; import java.util.Arrays; @@ -24,6 +25,7 @@ import java.util.Map; import java.util.Map.Entry; import java.util.Optional; +import java.util.function.IntFunction; import org.springframework.beans.PropertyAccessor; import org.springframework.beans.PropertyAccessorFactory; @@ -44,6 +46,7 @@ import org.springframework.http.converter.HttpMessageNotReadableException; import org.springframework.lang.Nullable; import org.springframework.util.Assert; +import org.springframework.util.ClassUtils; import org.springframework.util.ObjectUtils; import com.fasterxml.jackson.databind.JsonNode; @@ -264,7 +267,11 @@ T doMerge(ObjectNode root, T target, ObjectMapper mapper) throws Exception { if (child.isArray()) { - if (handleArray(child, it, mapper, property.getTypeInformation())) { + IntFunction rawValues = index -> readRawCollectionElement(property.getComponentType(), fieldName, index, + root, + mapper); + + if (handleArray(child, it, mapper, property.getTypeInformation(), rawValues)) { i.remove(); } @@ -304,6 +311,16 @@ T doMerge(ObjectNode root, T target, ObjectMapper mapper) throws Exception { return mapper.readerForUpdating(target).readValue(root); } + private static Object readRawCollectionElement(Class elementType, String fieldName, int index, JsonNode root, + ObjectMapper mapper) { + + try { + return mapper.readerFor(elementType).at("/" + fieldName + "/" + index).readValue(root); + } catch (IOException o_O) { + throw new RuntimeException(o_O); + } + } + /** * Handles the given {@link JsonNode} by treating it as {@link ArrayNode} and the given source value as * {@link Collection}-like value. Looks up the actual type to handle from the potentially available first element, @@ -316,7 +333,8 @@ T doMerge(ObjectNode root, T target, ObjectMapper mapper) throws Exception { * @return * @throws Exception */ - private boolean handleArray(JsonNode node, Object source, ObjectMapper mapper, TypeInformation collectionType) { + private boolean handleArray(JsonNode node, Object source, ObjectMapper mapper, TypeInformation collectionType, + IntFunction rawValues) { Collection collection = ifCollection(source); @@ -324,7 +342,8 @@ private boolean handleArray(JsonNode node, Object source, ObjectMapper mapper, T return false; } - return execute(() -> handleArrayNode((ArrayNode) node, collection, mapper, collectionType.getComponentType())); + return execute( + () -> handleArrayNode((ArrayNode) node, collection, mapper, collectionType.getComponentType(), rawValues)); } /** @@ -337,27 +356,47 @@ private boolean handleArray(JsonNode node, Object source, ObjectMapper mapper, T * @return whether an object merge has been applied to the {@link ArrayNode}. */ private boolean handleArrayNode(ArrayNode array, Collection collection, ObjectMapper mapper, - TypeInformation componentType) throws Exception { + TypeInformation componentType, IntFunction rawValues) throws Exception { Assert.notNull(array, "ArrayNode must not be null"); Assert.notNull(collection, "Source collection must not be null"); Assert.notNull(mapper, "ObjectMapper must not be null"); + // Empty collection? Primitive? Enum? No need to merge. + if (array.isEmpty() + || collection.isEmpty() + || ClassUtils.isPrimitiveOrWrapper(componentType.getType()) + || componentType.getType().isEnum()) { + return false; + } + // We need an iterator for the original collection. // We might modify it but we want to keep iterating over the original collection. Iterator value = new ArrayList(collection).iterator(); boolean nestedObjectFound = false; + int i = 0; for (JsonNode jsonNode : array) { + int current = i++; + + // We need to append new elements if (!value.hasNext()) { + + nestedObjectFound = true; + + // Use pre-read values if available. Deserialize node otherwise. + collection.add(rawValues != null + ? rawValues.apply(current) + : mapper.treeToValue(jsonNode, componentType.getType())); + break; } Object next = value.next(); if (ArrayNode.class.isInstance(jsonNode)) { - return handleArray(jsonNode, next, mapper, getTypeToMap(value, componentType)); + return handleArray(jsonNode, next, mapper, getTypeToMap(value, componentType), null); } if (ObjectNode.class.isInstance(jsonNode)) { @@ -410,7 +449,7 @@ private void doMergeNestedMap(Map source, ObjectNode node, Objec } else if (value instanceof ArrayNode && sourceValue != null) { - handleArray(value, sourceValue, mapper, getTypeToMap(sourceValue, typeToMap)); + handleArray(value, sourceValue, mapper, getTypeToMap(sourceValue, typeToMap), null); } else { 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 11ed30fad..db77867e2 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 @@ -107,6 +107,7 @@ void setUp() { mappingContext.getPersistentEntity(Apple.class); mappingContext.getPersistentEntity(Pear.class); mappingContext.getPersistentEntity(WithCustomMappedPrimitiveCollection.class); + mappingContext.getPersistentEntity(BugModel.class); mappingContext.afterPropertiesSet(); this.entities = new PersistentEntities(Collections.singleton(mappingContext)); @@ -678,6 +679,26 @@ void nestedEntitiesAreCreatedWhenMissingForPut() throws Exception { assertThat(result.inner.hidden).isNull(); } + @Test + void deserializesNewNestedEntitiesCorrectly() throws Exception { + + var mapper = new ObjectMapper(); + var node = mapper.readTree("{ \"list\" : [ { \"value\" : \"Foo\" }, { \"value\" : \"Bar\" }] }"); + + var nested = new BugModel.NestedModel(); + nested.value = "FooBar"; + + var model = new BugModel(); + model.list = new ArrayList<>(); + model.list.add(nested); + + var result = reader.doMerge((ObjectNode) node, model, mapper); + + assertThat(result.list) + .extracting(it -> it.value) + .containsExactly("Foo", "Bar"); + } + @SuppressWarnings("unchecked") private static T as(Object source, Class type) { @@ -1040,4 +1061,14 @@ public Long deserialize(JsonParser p, DeserializationContext ctxt) throws IOExce } } } + + // GH-2287 + static class BugModel { + + public List list; + + static class NestedModel { + public String value; + } + } }