DATAREST-956 - Fixed handling of collection element addition and removal for PUT requests.

DomainObjectMerger now properly adds and removes elements to and from collections.

Original pull request: #245.

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

@@ -19,6 +19,7 @@ import lombok.NonNull;
 import lombok.RequiredArgsConstructor;
 
 import java.io.InputStream;
+import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
@@ -48,6 +49,7 @@ import com.fasterxml.jackson.databind.node.ObjectNode;
  * @author Oliver Gierke
  * @author Mark Paluch
  * @author Craig Andrews
+ * @author Mathias Düsterhöft
  * @since 2.2
  */
 @RequiredArgsConstructor
@@ -175,7 +177,7 @@ public class DomainObjectReader {
 
 			if (child.isArray()) {
 
-				boolean nestedObjectFound = handleArrayNode((ArrayNode) child, asCollection(rawValue), mapper);
+				boolean nestedObjectFound = handleArrayNode((ArrayNode) child, asCollection(rawValue), property.getComponentType(), mapper);
 
 				if (nestedObjectFound) {
 					i.remove();
@@ -224,29 +226,35 @@ public class DomainObjectReader {
 	 * 
 	 * @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}.
+	 * @param componentType the item type of the collection
+	 * @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)
+	private boolean handleArrayNode(ArrayNode array, Collection<Object> collection, Class<?> componentType, 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();
+		//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;
 
 		for (JsonNode jsonNode : array) {
 
 			if (!value.hasNext()) {
-				return nestedObjectFound;
+				if (componentType != null) {
+					collection.add(mapper.treeToValue(jsonNode, componentType));
+					continue;
+				}
 			}
 
 			Object next = value.next();
 
 			if (ArrayNode.class.isInstance(jsonNode)) {
-				return handleArrayNode(array, asCollection(next), mapper);
+				Collection<Object> nestedCollection = asCollection(next);
+				Iterator<Object> iterator = nestedCollection.iterator();
+				return handleArrayNode(array, nestedCollection, iterator.hasNext() ? iterator.next().getClass() : null, mapper);
 			}
 
 			if (ObjectNode.class.isInstance(jsonNode)) {
@@ -256,6 +264,11 @@ public class DomainObjectReader {
 			}
 		}
 
+		while (value.hasNext()) {
+			//there are more items in the collection than contained in the json node - remove it.
+			collection.remove(value.next());
+		}
+
 		return nestedObjectFound;
 	}
 
@@ -284,7 +297,9 @@ public class DomainObjectReader {
 			if (child instanceof ObjectNode && sourceValue != null) {
 				doMerge((ObjectNode) child, sourceValue, mapper);
 			} else if (child instanceof ArrayNode && sourceValue != null) {
-				handleArrayNode((ArrayNode) child, asCollection(sourceValue), mapper);
+				Collection<Object> nestedCollection = asCollection(sourceValue);
+				Iterator<Object> iterator = nestedCollection.iterator();
+				handleArrayNode((ArrayNode) child, nestedCollection, iterator.hasNext() ? iterator.next().getClass() : null, mapper);
 			} else {
 				source.put(entry.getKey(),
 						mapper.treeToValue(child, sourceValue == null ? Object.class : sourceValue.getClass()));

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

@@ -15,9 +15,13 @@
  */
 package org.springframework.data.rest.webmvc.json;
 
-import static org.hamcrest.CoreMatchers.*;
-import static org.junit.Assert.*;
-import static org.mockito.Mockito.*;
+import static org.hamcrest.CoreMatchers.instanceOf;
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.CoreMatchers.notNullValue;
+import static org.hamcrest.CoreMatchers.nullValue;
+import static org.hamcrest.CoreMatchers.sameInstance;
+import static org.junit.Assert.assertThat;
+import static org.mockito.Mockito.mock;
 
 import java.io.ByteArrayInputStream;
 import java.util.ArrayList;
@@ -55,11 +59,16 @@ import com.fasterxml.jackson.databind.PropertyNamingStrategy;
 import com.fasterxml.jackson.databind.node.ObjectNode;
 import com.google.common.base.Charsets;
 
+import javassist.runtime.Inner;
+import lombok.AllArgsConstructor;
+import lombok.NoArgsConstructor;
+
 /**
  * Unit tests for {@link DomainObjectReader}.
  * 
  * @author Oliver Gierke
  * @author Craig Andrews
+ * @author Mathias Düsterhöft
  */
 @RunWith(MockitoJUnitRunner.class)
 public class DomainObjectReaderUnitTests {
@@ -373,6 +382,55 @@ public class DomainObjectReaderUnitTests {
 		assertThat(result.inner.items.get(0).some, is("value"));
 	}
 
+	/**
+	 * @see DATAREST-956
+	 */
+	@Test
+	public void writesArrayWithAddedItemForPut() throws Exception {
+
+		Child inner = new Child();
+		inner.items = new ArrayList<Item>();
+		inner.items.add(new Item());
+
+		Parent source = new Parent();
+		source.inner = inner;
+
+		JsonNode node = new ObjectMapper().readTree("{ \"inner\" : { \"items\" : [ " +
+				"{ \"some\" : \"value1\" }," +
+				"{ \"some\" : \"value2\" }," +
+				"{ \"some\" : \"value3\" } ] } }");
+
+		Parent result = reader.readPut((ObjectNode) node, source, new ObjectMapper());
+
+		assertThat(result.inner.items.size(), is(3));
+		assertThat(result.inner.items.get(0).some, is("value1"));
+		assertThat(result.inner.items.get(1).some, is("value2"));
+		assertThat(result.inner.items.get(2).some, is("value3"));
+	}
+
+	/**
+	 * @see DATAREST-956
+	 */
+	@Test
+	public void writesArrayWithRemovedItemForPut() throws Exception {
+
+		Child inner = new Child();
+		inner.items = new ArrayList<Item>();
+		inner.items.add(new Item("test1"));
+		inner.items.add(new Item("test2"));
+		inner.items.add(new Item("test3"));
+
+		Parent source = new Parent();
+		source.inner = inner;
+
+		JsonNode node = new ObjectMapper().readTree("{ \"inner\" : { \"items\" : [ { \"some\" : \"value\" } ] } }");
+
+		Parent result = reader.readPut((ObjectNode) node, source, new ObjectMapper());
+
+		assertThat(result.inner.items.size(), is(1));
+		assertThat(result.inner.items.get(0).some, is("value"));
+	}
+
 	@JsonAutoDetect(fieldVisibility = Visibility.ANY)
 	static class TypeWithGenericMap {
 
@@ -440,6 +498,8 @@ public class DomainObjectReaderUnitTests {
 	}
 
 	@JsonAutoDetect(fieldVisibility = Visibility.ANY)
+	@NoArgsConstructor
+	@AllArgsConstructor
 	static class Item {
 		String some;
 	}