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.

Author: odrotbohm

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,10 @@ <T> T doMerge(ObjectNode root, T target, ObjectMapper mapper) throws Exception {
 
 				if (child.isArray()) {
 
-					if (handleArray(child, it, mapper, property.getTypeInformation())) {
+					IntFunction<Object> rawValues = index -> readRawCollectionElement(property.getComponentType(), fieldName,
+							index, root, mapper);
+
+					if (handleArray(child, it, mapper, property.getTypeInformation(), rawValues)) {
 						i.remove();
 					}
 
@@ -304,6 +310,16 @@ <T> 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,15 +332,17 @@ <T> 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<Object> rawValues) {
 
 		Collection<Object> collection = ifCollection(source);
 
 		if (collection == null) {
 			return false;
 		}
 
-		return execute(() -> handleArrayNode((ArrayNode) node, collection, mapper, collectionType.getComponentType()));
+		return execute(
+				() -> handleArrayNode((ArrayNode) node, collection, mapper, collectionType.getComponentType(), rawValues));
 	}
 
 	/**
@@ -337,27 +355,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<Object> collection, ObjectMapper mapper,
-			TypeInformation<?> componentType) throws Exception {
+			TypeInformation<?> componentType, IntFunction<Object> 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<Object> value = new ArrayList<Object>(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 +448,7 @@ private void doMergeNestedMap(Map<Object, Object> 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 {
 
@@ -685,8 +723,8 @@ public void doWithPersistentProperty(PersistentProperty<?> property) {
 				result = mergeCollections(property, sourceValue, targetValue, mapper);
 			} else if (property.isEntity()) {
 
-                result = targetValue.isPresent() ? targetValue.flatMap(t -> sourceValue.map(s -> mergeForPut(s, t, mapper)))
-                        : sourceValue;
+				result = targetValue.isPresent() ? targetValue.flatMap(t -> sourceValue.map(s -> mergeForPut(s, t, mapper)))
+						: sourceValue;
 			} else {
 				result = sourceValue;
 			}

spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/DomainObjectReaderUnitTests.java

@@ -49,6 +49,7 @@
 import org.springframework.data.mapping.context.PersistentEntities;
 import org.springframework.data.rest.core.config.RepositoryRestConfiguration;
 import org.springframework.data.rest.core.mapping.ResourceMappings;
+import org.springframework.data.rest.webmvc.json.DomainObjectReaderUnitTests.BugModel.NestedModel;
 import org.springframework.data.rest.webmvc.mapping.Associations;
 
 import com.fasterxml.jackson.annotation.JsonAutoDetect;
@@ -113,6 +114,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));
@@ -682,6 +684,26 @@ void nestedEntitiesAreCreatedWhenMissingForPut() throws Exception {
 		assertThat(result.inner.hidden).isNull();
 	}
 
+	@Test
+	void deserializesNewNestedEntitiesCorrectly() throws Exception {
+
+		ObjectMapper mapper = new ObjectMapper();
+		JsonNode node = mapper.readTree("{ \"list\" : [ { \"value\" : \"Foo\" }, { \"value\" : \"Bar\" }] }");
+
+		NestedModel nested = new BugModel.NestedModel();
+		nested.value = "FooBar";
+
+		BugModel model = new BugModel();
+		model.list = new ArrayList<>();
+		model.list.add(nested);
+
+		BugModel result = reader.doMerge((ObjectNode) node, model, mapper);
+
+		assertThat(result.list)
+				.extracting(it -> it.value)
+				.containsExactly("Foo", "Bar");
+	}
+
 	@SuppressWarnings("unchecked")
 	private static <T> T as(Object source, Class<T> type) {
 
@@ -968,4 +990,14 @@ public Long deserialize(JsonParser p, DeserializationContext ctxt) throws IOExce
 			}
 		}
 	}
+
+	// GH-2287
+	static class BugModel {
+
+		public List<NestedModel> list;
+
+		static class NestedModel {
+			public String value;
+		}
+	}
 }