From 67d6a1aa16feddf38e553d9688fc6af0c0f53786 Mon Sep 17 00:00:00 2001 From: Oliver Gierke Date: Mon, 31 Oct 2016 13:40:37 +0100 Subject: [PATCH] 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. --- .../rest/webmvc/json/DomainObjectReader.java | 103 +++++++++++++++--- .../json/DomainObjectReaderUnitTests.java | 35 ++++++ 2 files changed, 125 insertions(+), 13 deletions(-) diff --git a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/DomainObjectReader.java b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/DomainObjectReader.java index 0ed750bb2..abbac66bc 100644 --- a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/DomainObjectReader.java +++ b/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 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 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) nested, objectNode, mapper); + doMergeNestedMap((Map) 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 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 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 asCollection(Object source) { + + if (source == null) { + return Collections.emptyList(); + } + + if (source instanceof Collection) { + return (Collection) 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(); diff --git a/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/DomainObjectReaderUnitTests.java b/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/DomainObjectReaderUnitTests.java index 5fb4600ab..76297b471 100644 --- a/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/DomainObjectReaderUnitTests.java +++ b/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 phones = new ArrayList(); + } + + static class Phone { + + public Calendar creationDate; + public String label; + } }