DATAREST-931 - DomainObjectMerger now handles arrays with complex objects correctly.

We now explicitly manually merge array nodes that contain complex objects. Previously arrays would've been skipped and the subsequent Jackson update would wipe out all properties not contained in the original document even on PATCH requests.

Author: odrotbohm

spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/config/JsonPatchHandler.java

@@ -123,7 +123,7 @@ <T> T applyPatch(InputStream source, T target) throws Exception {
 			}
 		}
 
-		return reader.merge((ObjectNode) patchedNode, target, mapper);
+		return mapper.readerForUpdating(target).readValue(patchedNode);
 	}
 
 	<T> T applyMergePatch(InputStream source, T existingObject) throws Exception {

spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/DomainObjectReader.java

@@ -16,7 +16,10 @@
 package org.springframework.data.rest.webmvc.json;
 
 import java.io.InputStream;
+import java.util.Arrays;
 import java.util.HashMap;
+import java.util.Collection;
+import java.util.Collections;
 import java.util.Iterator;
 import java.util.Map;
 import java.util.Map.Entry;
@@ -35,6 +38,7 @@
 import com.fasterxml.jackson.databind.JsonNode;
 import com.fasterxml.jackson.databind.ObjectMapper;
 import com.fasterxml.jackson.databind.introspect.BasicClassIntrospector;
+import com.fasterxml.jackson.databind.node.ArrayNode;
 import com.fasterxml.jackson.databind.introspect.BeanPropertyDefinition;
 import com.fasterxml.jackson.databind.introspect.ClassIntrospector;
 import com.fasterxml.jackson.databind.node.ObjectNode;
@@ -155,6 +159,7 @@ public <T> T merge(ObjectNode source, T target, ObjectMapper mapper) {
 	 * @return
 	 * @throws Exception
 	 */
+	@SuppressWarnings("unchecked")
 	private <T> T doMerge(ObjectNode root, T target, ObjectMapper mapper) throws Exception {
 
 		Assert.notNull(root, "Root ObjectNode must not be null!");
@@ -173,29 +178,34 @@ private <T> T doMerge(ObjectNode root, T target, ObjectMapper mapper) throws Exc
 
 			Entry<String, JsonNode> entry = i.next();
 			JsonNode child = entry.getValue();
+			String fieldName = entry.getKey();
 
-			if (child.isArray()) {
+			if (!mappedProperties.hasPersistentPropertyForField(fieldName)) {
+				i.remove();
 				continue;
 			}
 
-			String fieldName = entry.getKey();
+			PersistentProperty<?> property = mappedProperties.getPersistentProperty(fieldName);
+			PersistentPropertyAccessor accessor = entity.getPropertyAccessor(target);
+			Object rawValue = accessor.getProperty(property);
+
+			if (child.isArray()) {
+
+				boolean nestedObjectFound = handleArrayNode((ArrayNode) child, asCollection(rawValue), mapper);
+
+				if (nestedObjectFound) {
+					i.remove();
+				}
 
-			if (!mappedProperties.hasPersistentPropertyForField(fieldName)) {
-				i.remove();
 				continue;
 			}
 
 			if (child.isObject()) {
 
-				PersistentProperty<?> property = mappedProperties.getPersistentProperty(fieldName);
-
 				if (associationLinks.isLinkableAssociation(property)) {
 					continue;
 				}
 
-				PersistentPropertyAccessor accessor = entity.getPropertyAccessor(target);
-				Object nested = accessor.getProperty(property);
-
 				ObjectNode objectNode = (ObjectNode) child;
 
 				if (property.isMap()) {
@@ -205,7 +215,7 @@ private <T> T doMerge(ObjectNode root, T target, ObjectMapper mapper) throws Exc
 						continue;
 					}
 
-					doMergeNestedMap((Map<String, Object>) nested, objectNode, mapper);
+					doMergeNestedMap((Map<String, Object>) rawValue, objectNode, mapper);
 
 					// Remove potentially emptied Map as values have been handled recursively
 					if (!objectNode.fieldNames().hasNext()) {
@@ -215,15 +225,55 @@ private <T> T doMerge(ObjectNode root, T target, ObjectMapper mapper) throws Exc
 					continue;
 				}
 
-				if (nested != null && property.isEntity()) {
-					doMerge(objectNode, nested, mapper);
+				if (rawValue != null && property.isEntity()) {
+					doMerge(objectNode, rawValue, mapper);
 				}
 			}
 		}
 
 		return mapper.readerForUpdating(target).readValue(root);
 	}
 
+	/**
+	 * Applies the diff handling to {@link ArrayNode}s, potentially recursing into nested ones.
+	 * 
+	 * @param array the source {@link ArrayNode}m, must not be {@literal null}.
+	 * @param collection the actual collection values, must not be {@literal null}.
+	 * @param mapper the {@link ObjectMapper} to use, must not be {@literal null}.
+	 * @return whether an object merge has been applied to the {@link ArrayNode}.
+	 */
+	private boolean handleArrayNode(ArrayNode array, Collection<Object> collection, ObjectMapper mapper)
+			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!");
+
+		Iterator<Object> value = collection.iterator();
+		boolean nestedObjectFound = false;
+
+		for (JsonNode jsonNode : array) {
+
+			if (!value.hasNext()) {
+				return nestedObjectFound;
+			}
+
+			Object next = value.next();
+
+			if (ArrayNode.class.isInstance(jsonNode)) {
+				return handleArrayNode(array, asCollection(next), mapper);
+			}
+
+			if (ObjectNode.class.isInstance(jsonNode)) {
+
+				nestedObjectFound = true;
+				doMerge((ObjectNode) jsonNode, next, mapper);
+			}
+		}
+
+		return nestedObjectFound;
+	}
+
 	/**
 	 * Merges nested {@link Map} values for the given source {@link Map}, the {@link ObjectNode} and {@link ObjectMapper}.
 	 * 
@@ -253,11 +303,38 @@ private void doMergeNestedMap(Map<String, Object> source, ObjectNode node, Objec
 		}
 	}
 
+	/**
+	 * Returns the given source instance as {@link Collection}.
+	 * 
+	 * @param source can be {@literal null}.
+	 * @return
+	 */
+	@SuppressWarnings("unchecked")
+	private static Collection<Object> asCollection(Object source) {
+
+		if (source == null) {
+			return Collections.emptyList();
+		}
+
+		if (source instanceof Collection) {
+			return (Collection<Object>) source;
+		}
+
+		if (source.getClass().isArray()) {
+			return Arrays.asList((Object[]) source);
+		}
+
+		return Collections.singleton(source);
+	}
+
 	/**
 	 * Simple value object to capture a mapping of Jackson mapped field names and {@link PersistentProperty} instances.
+	 * 
+	 * @param source can be {@literal null}.
 	 *
 	 * @author Oliver Gierke
 	 */
+	@SuppressWarnings("unchecked")
 	static class MappedProperties {
 
 		private static final ClassIntrospector INTROSPECTOR = new BasicClassIntrospector();

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

@@ -18,9 +18,14 @@
 import static org.hamcrest.CoreMatchers.*;
 import static org.junit.Assert.*;
 
+import java.io.ByteArrayInputStream;
+import java.util.ArrayList;
+import java.util.Calendar;
 import java.util.Collections;
 import java.util.Date;
+import java.util.GregorianCalendar;
 import java.util.HashMap;
+import java.util.List;
 import java.util.Map;
 
 import org.junit.Before;
@@ -43,6 +48,7 @@
 import com.fasterxml.jackson.databind.ObjectMapper;
 import com.fasterxml.jackson.databind.PropertyNamingStrategy;
 import com.fasterxml.jackson.databind.node.ObjectNode;
+import com.google.common.base.Charsets;
 
 /**
  * Unit tests for {@link DomainObjectReader}.
@@ -65,6 +71,7 @@ public void setUp() {
 		mappingContext.getPersistentEntity(TypeWithGenericMap.class);
 		mappingContext.getPersistentEntity(VersionedType.class);
 		mappingContext.getPersistentEntity(SampleWithCreatedDate.class);
+		mappingContext.getPersistentEntity(User.class);
 		mappingContext.afterPropertiesSet();
 
 		PersistentEntities entities = new PersistentEntities(Collections.singleton(mappingContext));
@@ -190,6 +197,23 @@ public void doesNotApplyInputToReadOnlyFields() throws Exception {
 		assertThat(reader.readPut(node, sample, mapper).createdDate, is(reference));
 	}
 
+	@Test
+	public void readsPatchForEntityNestedInCollection() throws Exception {
+
+		Phone phone = new Phone();
+		phone.creationDate = new GregorianCalendar();
+
+		User user = new User();
+		user.phones.add(phone);
+
+		ByteArrayInputStream source = new ByteArrayInputStream(
+				"{ \"phones\" : [ { \"label\" : \"some label\" } ] }".getBytes(Charsets.UTF_8));
+
+		User result = reader.read(source, user, new ObjectMapper());
+
+		assertThat(result.phones.get(0).creationDate, is(notNullValue()));
+	}
+
 	@JsonAutoDetect(fieldVisibility = Visibility.ANY)
 	static class SampleUser {
 
@@ -239,4 +263,15 @@ static class SampleWithCreatedDate {
 		@ReadOnlyProperty //
 		Date createdDate;
 	}
+
+	static class User {
+
+		public List<Phone> phones = new ArrayList<Phone>();
+	}
+
+	static class Phone {
+
+		public Calendar creationDate;
+		public String label;
+	}
 }