DATAREST-965 - Switched to property based application of PUT requests.

Jacksons ObjectMapper.readerForUpdate(…) unfortunately doesn't handle nested objects properly. We already have a manual merge process in place for PATCH requests but tweaking that to also handle PUT requests gracefully caused more complexity than anticipated.

We now switched to an object based merge so that we can read in the source JSON structure into a new object and then merge the objects.

Related pull request: #247.

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

@@ -22,20 +22,28 @@ import java.io.InputStream;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.Iterator;
 import java.util.Map;
 import java.util.Map.Entry;
 
+import org.springframework.beans.PropertyAccessor;
+import org.springframework.beans.PropertyAccessorFactory;
+import org.springframework.core.CollectionFactory;
+import org.springframework.core.convert.ConversionService;
+import org.springframework.core.convert.support.DefaultConversionService;
 import org.springframework.data.mapping.PersistentEntity;
 import org.springframework.data.mapping.PersistentProperty;
 import org.springframework.data.mapping.PersistentPropertyAccessor;
 import org.springframework.data.mapping.SimplePropertyHandler;
 import org.springframework.data.mapping.context.PersistentEntities;
+import org.springframework.data.mapping.model.ConvertingPropertyAccessor;
 import org.springframework.data.rest.webmvc.mapping.Associations;
 import org.springframework.data.util.ClassTypeInformation;
 import org.springframework.data.util.TypeInformation;
 import org.springframework.http.converter.HttpMessageNotReadableException;
 import org.springframework.util.Assert;
+import org.springframework.util.ObjectUtils;
 
 import com.fasterxml.jackson.databind.JsonNode;
 import com.fasterxml.jackson.databind.ObjectMapper;
@@ -88,6 +96,7 @@ public class DomainObjectReader {
 	 * @param mapper
 	 * @return
 	 */
+	@SuppressWarnings("unchecked")
 	public <T> T readPut(final ObjectNode source, T target, final ObjectMapper mapper) {
 
 		Assert.notNull(source, "ObjectNode must not be null!");
@@ -100,8 +109,50 @@ public class DomainObjectReader {
 
 		Assert.notNull(entity, "No PersistentEntity found for ".concat(type.getName()).concat("!"));
 
+		try {
+
+			Object intermediate = mapper.readerFor(target.getClass()).readValue(source);
+			return (T) mergeForPut(intermediate, target, mapper);
+
+		} catch (Exception o_O) {
+			throw new HttpMessageNotReadableException("Could not read payload!", o_O);
+		}
+	}
+
+	/**
+	 * Merges the state of given source object onto the target one preserving PUT semantics.
+	 * 
+	 * @param source can be {@literal null}.
+	 * @param target can be {@literal null}.
+	 * @param mapper must not be {@literal null}.
+	 * @return
+	 */
+	private <T> T mergeForPut(T source, T target, final ObjectMapper mapper) {
+
+		Assert.notNull(mapper, "ObjectMapper must not be null!");
+
+		if (target == null || source == null) {
+			return source;
+		}
+
+		Class<? extends Object> type = target.getClass();
+
+		final PersistentEntity<?, ?> entity = entities.getPersistentEntity(type);
+
+		if (entity == null) {
+			return source;
+		}
+
+		Assert.notNull(entity, "No PersistentEntity found for ".concat(type.getName()).concat("!"));
+
 		final MappedProperties properties = MappedProperties.fromJacksonProperties(entity, mapper);
 
+		ConversionService conversionService = new DefaultConversionService();
+		final PersistentPropertyAccessor targetAccessor = entity.getPropertyAccessor(target);
+		final ConvertingPropertyAccessor convertingAccessor = new ConvertingPropertyAccessor(targetAccessor,
+				conversionService);
+		final PersistentPropertyAccessor sourceAccessor = entity.getPropertyAccessor(source);
+
 		entity.doWithProperties(new SimplePropertyHandler() {
 
 			/*
@@ -115,18 +166,50 @@ public class DomainObjectReader {
 					return;
 				}
 
-				String mappedName = properties.getMappedName(property);
-
-				boolean isMappedProperty = mappedName != null;
-				boolean noValueInSource = !source.has(mappedName);
-
-				if (isMappedProperty && noValueInSource) {
-					source.putNull(mappedName);
+				if (!properties.isMappedProperty(property)) {
+					return;
 				}
+
+				Object sourceValue = sourceAccessor.getProperty(property);
+				Object targetValue = targetAccessor.getProperty(property);
+				Object result = null;
+
+				if (property.isMap()) {
+					result = mergeMaps(property, sourceValue, targetValue, mapper);
+				} else if (property.isCollectionLike()) {
+					result = mergeCollections(property, sourceValue, targetValue, mapper);
+				} else if (property.isEntity()) {
+					result = mergeForPut(sourceValue, targetValue, mapper);
+				} else {
+					result = sourceValue;
+				}
+
+				convertingAccessor.setProperty(property, result);
 			}
 		});
 
-		return merge(source, target, mapper);
+		// Need to copy unmapped properties as the PersistentProperty model currently does not contain any transient
+		// properties
+		copyRemainingProperties(properties, source, target);
+
+		return target;
+	}
+
+	/**
+	 * Copies the unmapped properties of the given {@link MappedProperties} from the source object to the target instance.
+	 * 
+	 * @param properties must not be {@literal null}.
+	 * @param source must not be {@literal null}.
+	 * @param target must not be {@literal null}.
+	 */
+	private static void copyRemainingProperties(MappedProperties properties, Object source, Object target) {
+
+		PropertyAccessor sourceAccessor = PropertyAccessorFactory.forDirectFieldAccess(source);
+		PropertyAccessor targetAccessor = PropertyAccessorFactory.forDirectFieldAccess(target);
+
+		for (String property : properties.getSpringDataUnmappedProperties()) {
+			targetAccessor.setPropertyValue(property, sourceAccessor.getPropertyValue(property));
+		}
 	}
 
 	public <T> T merge(ObjectNode source, T target, ObjectMapper mapper) {
@@ -148,7 +231,7 @@ public class DomainObjectReader {
 	 * @throws Exception
 	 */
 	@SuppressWarnings("unchecked")
-	private <T> T doMerge(ObjectNode root, T target, ObjectMapper mapper) throws Exception {
+	<T> T doMerge(ObjectNode root, T target, ObjectMapper mapper) throws Exception {
 
 		Assert.notNull(root, "Root ObjectNode must not be null!");
 		Assert.notNull(target, "Target object instance must not be null!");
@@ -351,6 +434,72 @@ public class DomainObjectReader {
 		}
 	}
 
+	@SuppressWarnings("unchecked")
+	private Map<Object, Object> mergeMaps(PersistentProperty<?> property, Object source, Object target,
+			ObjectMapper mapper) {
+
+		Map<Object, Object> sourceMap = (Map<Object, Object>) source;
+
+		if (sourceMap == null) {
+			return null;
+		}
+
+		Map<Object, Object> targetMap = (Map<Object, Object>) target;
+		Map<Object, Object> result = targetMap == null ? CollectionFactory.createMap(Map.class, sourceMap.size())
+				: CollectionFactory.createApproximateMap(targetMap, sourceMap.size());
+
+		for (Entry<Object, Object> entry : sourceMap.entrySet()) {
+
+			Object targetValue = targetMap == null ? null : targetMap.get(entry.getKey());
+			result.put(entry.getKey(), mergeForPut(entry.getValue(), targetValue, mapper));
+		}
+
+		return result;
+	}
+
+	private Collection<Object> mergeCollections(PersistentProperty<?> property, Object source, Object target,
+			ObjectMapper mapper) {
+
+		Collection<Object> sourceCollection = asCollection(source);
+
+		if (sourceCollection == null) {
+			return null;
+		}
+
+		Collection<Object> targetCollection = asCollection(target);
+		Collection<Object> result = targetCollection == null
+				? CollectionFactory.createCollection(Collection.class, sourceCollection.size())
+				: CollectionFactory.createApproximateCollection(targetCollection, sourceCollection.size());
+
+		Iterator<Object> sourceIterator = sourceCollection.iterator();
+		Iterator<Object> targetIterator = targetCollection == null ? Collections.emptyIterator()
+				: targetCollection.iterator();
+
+		while (sourceIterator.hasNext()) {
+
+			Object sourceElement = sourceIterator.next();
+			Object targetElement = targetIterator.hasNext() ? targetIterator.next() : null;
+
+			result.add(mergeForPut(sourceElement, targetElement, mapper));
+		}
+
+		return result;
+	}
+
+	@SuppressWarnings("unchecked")
+	private static Collection<Object> asCollection(Object source) {
+
+		if (source == null) {
+			return null;
+		} else if (source instanceof Collection) {
+			return (Collection<Object>) source;
+		} else if (source.getClass().isArray()) {
+			return Arrays.asList(ObjectUtils.toObjectArray(source));
+		} else {
+			return Collections.singleton(source);
+		}
+	}
+
 	/**
 	 * Returns the given source instance as {@link Collection} or creates a new one for the given type.
 	 * 

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

@@ -15,8 +15,13 @@
  */
 package org.springframework.data.rest.webmvc.json;
 
+import java.util.ArrayList;
+import java.util.Collections;
 import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
 import java.util.Map;
+import java.util.Set;
 
 import org.springframework.data.mapping.PersistentEntity;
 import org.springframework.data.mapping.PersistentProperty;
@@ -38,8 +43,9 @@ class MappedProperties {
 
 	private static final ClassIntrospector INTROSPECTOR = new BasicClassIntrospector();
 
-	private final Map<PersistentProperty<?>, String> propertyToFieldName;
+	private final Map<PersistentProperty<?>, BeanPropertyDefinition> propertyToFieldName;
 	private final Map<String, PersistentProperty<?>> fieldNameToProperty;
+	private final Set<BeanPropertyDefinition> unmappedProperties;
 
 	/**
 	 * Creates a new {@link MappedProperties} instance for the given {@link PersistentEntity} and {@link BeanDescription}.
@@ -52,16 +58,19 @@ class MappedProperties {
 		Assert.notNull(entity, "Entity must not be null!");
 		Assert.notNull(description, "BeanDescription must not be null!");
 
-		this.propertyToFieldName = new HashMap<PersistentProperty<?>, String>();
+		this.propertyToFieldName = new HashMap<PersistentProperty<?>, BeanPropertyDefinition>();
 		this.fieldNameToProperty = new HashMap<String, PersistentProperty<?>>();
+		this.unmappedProperties = new HashSet<BeanPropertyDefinition>();
 
 		for (BeanPropertyDefinition property : description.findProperties()) {
 
 			PersistentProperty<?> persistentProperty = entity.getPersistentProperty(property.getInternalName());
 
 			if (persistentProperty != null) {
-				propertyToFieldName.put(persistentProperty, property.getName());
+				propertyToFieldName.put(persistentProperty, property);
 				fieldNameToProperty.put(property.getName(), persistentProperty);
+			} else {
+				unmappedProperties.add(property);
 			}
 		}
 	}
@@ -89,7 +98,7 @@ class MappedProperties {
 
 		Assert.notNull(property, "PersistentProperty must not be null!");
 
-		return propertyToFieldName.get(property);
+		return propertyToFieldName.get(property).getName();
 	}
 
 	/**
@@ -113,4 +122,37 @@ class MappedProperties {
 
 		return fieldNameToProperty.get(fieldName);
 	}
+
+	/**
+	 * Returns all properties only known to Jackson.
+	 * 
+	 * @return the names of all properties that are not known to Spring Data but appear in the Jackson metamodel.
+	 */
+	public Iterable<String> getSpringDataUnmappedProperties() {
+
+		if (unmappedProperties.isEmpty()) {
+			return Collections.emptySet();
+		}
+
+		List<String> result = new ArrayList<String>(unmappedProperties.size());
+
+		for (BeanPropertyDefinition definitions : unmappedProperties) {
+			result.add(definitions.getInternalName());
+		}
+
+		return result;
+	}
+
+	/**
+	 * Returns whether the given {@link PersistentProperty} is mapped, i.e. known to both Jackson and Spring Data.
+	 * 
+	 * @param property must not be {@literal null}.
+	 * @return
+	 */
+	public boolean isMappedProperty(PersistentProperty<?> property) {
+
+		Assert.notNull(property, "PersistentProperty must not be null!");
+
+		return propertyToFieldName.containsKey(property);
+	}
 }

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

@@ -68,6 +68,7 @@ import com.google.common.base.Charsets;
  * @author Oliver Gierke
  * @author Craig Andrews
  * @author Mathias Düsterhöft
+ * @author Ken Dombeck
  */
 @RunWith(MockitoJUnitRunner.class)
 public class DomainObjectReaderUnitTests {
@@ -271,7 +272,7 @@ public class DomainObjectReaderUnitTests {
 
 		JsonNode node = new ObjectMapper().readTree("{ \"inner\" : { \"name\" : \"new inner name\" } }");
 
-		Outer result = reader.merge((ObjectNode) node, outer, new ObjectMapper());
+		Outer result = reader.doMerge((ObjectNode) node, outer, new ObjectMapper());
 
 		assertThat(result, is(sameInstance(outer)));
 		assertThat(result.prop, is("else"));
@@ -390,13 +391,41 @@ public class DomainObjectReaderUnitTests {
 		assertThat(iterator.next().get("some"), is((Object) "otherValue"));
 	}
 
-	@Test
-	public void testname() throws Exception {
+	@Test // DATAREST-965
+	public void writesObjectWithRemovedItemsForPut() throws Exception {
 
-		ObjectMapper mapper = new ObjectMapper();
-		JsonNode node = mapper.readTree("\"asd\"");
+		Child inner = new Child();
+		inner.items = new ArrayList<Item>();
+		inner.items.add(new Item("test1"));
+		inner.items.add(new Item("test2"));
 
-		assertThat(mapper.treeToValue(node, Object.class), is((Object) "asd"));
+		Parent source = new Parent();
+		source.inner = inner;
+
+		JsonNode node = new ObjectMapper().readTree("{ \"inner\" : { \"object\" : \"value\" } }");
+
+		Parent result = reader.readPut((ObjectNode) node, source, new ObjectMapper());
+
+		assertThat(result.inner.items, is(nullValue()));
+		assertThat((String) result.inner.object, is("value"));
+	}
+
+	@Test // DATAREST-965
+	public void writesArrayWithRemovedObjectForPut() throws Exception {
+
+		Child inner = new Child();
+		inner.object = "value";
+
+		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"));
+		assertThat(result.inner.object, is(nullValue()));
 	}
 
 	@Test // DATAREST-986
@@ -427,9 +456,12 @@ public class DomainObjectReaderUnitTests {
 		Map<String, SampleUser> relatedUsers;
 
 		public SampleUser(String name, String password) {
+
 			this.name = name;
 			this.password = password;
 		}
+
+		protected SampleUser() {}
 	}
 
 	// DATAREST-556
@@ -442,6 +474,8 @@ public class DomainObjectReaderUnitTests {
 			this.firstName = firstName;
 			this.lastName = lastName;
 		}
+
+		protected Person() {}
 	}
 
 	@JsonAutoDetect(fieldVisibility = Visibility.ANY)