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.

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

@@ -19,7 +19,10 @@ import lombok.NonNull;
 import lombok.RequiredArgsConstructor;
 
 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;
@@ -37,6 +40,7 @@ import com.fasterxml.jackson.databind.BeanDescription;
 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;
@@ -143,6 +147,7 @@ public class DomainObjectReader {
 	 * @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!");
@@ -161,11 +166,6 @@ public class DomainObjectReader {
 
 			Entry<String, JsonNode> entry = i.next();
 			JsonNode child = entry.getValue();
-
-			if (child.isArray()) {
-				continue;
-			}
-
 			String fieldName = entry.getKey();
 
 			if (!mappedProperties.hasPersistentPropertyForField(fieldName)) {
@@ -173,17 +173,27 @@ public class DomainObjectReader {
 				continue;
 			}
 
-			if (child.isObject()) {
+			PersistentProperty<?> property = mappedProperties.getPersistentProperty(fieldName);
+			PersistentPropertyAccessor accessor = entity.getPropertyAccessor(target);
+			Object rawValue = accessor.getProperty(property);
 
-				PersistentProperty<?> property = mappedProperties.getPersistentProperty(fieldName);
+			if (child.isArray()) {
+
+				boolean nestedObjectFound = handleArrayNode((ArrayNode) child, asCollection(rawValue), mapper);
+
+				if (nestedObjectFound) {
+					i.remove();
+				}
+
+				continue;
+			}
+
+			if (child.isObject()) {
 
 				if (associationLinks.isLinkableAssociation(property)) {
 					continue;
 				}
 
-				PersistentPropertyAccessor accessor = entity.getPropertyAccessor(target);
-				Object nested = accessor.getProperty(property);
-
 				ObjectNode objectNode = (ObjectNode) child;
 
 				if (property.isMap()) {
@@ -193,7 +203,7 @@ public class DomainObjectReader {
 						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()) {
@@ -203,8 +213,8 @@ public class DomainObjectReader {
 					continue;
 				}
 
-				if (nested != null && property.isEntity()) {
-					doMerge(objectNode, nested, mapper);
+				if (rawValue != null && property.isEntity()) {
+					doMerge(objectNode, rawValue, mapper);
 				}
 			}
 		}
@@ -212,6 +222,46 @@ public class DomainObjectReader {
 		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}.
 	 * 
@@ -241,11 +291,38 @@ public class DomainObjectReader {
 		}
 	}
 
+	/**
+	 * 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

@@ -19,9 +19,14 @@ import static org.hamcrest.CoreMatchers.*;
 import static org.junit.Assert.*;
 import static org.mockito.Mockito.*;
 
+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;
@@ -46,6 +51,7 @@ import com.fasterxml.jackson.databind.JsonNode;
 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}.
@@ -68,6 +74,7 @@ public class DomainObjectReaderUnitTests {
 		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));
@@ -193,6 +200,23 @@ public class DomainObjectReaderUnitTests {
 		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 {
 
@@ -242,4 +266,15 @@ public class DomainObjectReaderUnitTests {
 		@ReadOnlyProperty //
 		Date createdDate;
 	}
+
+	static class User {
+
+		public List<Phone> phones = new ArrayList<Phone>();
+	}
+
+	static class Phone {
+
+		public Calendar creationDate;
+		public String label;
+	}
 }