From 436a1fb9b90dfe0082c491b4c0eb39428d588e23 Mon Sep 17 00:00:00 2001 From: Oliver Gierke Date: Wed, 22 Jul 2015 17:46:01 +0200 Subject: [PATCH] DATAREST-605 - Fixed processing of Maps in DomainObjectReader. We now post-process Map values recursively to make sure they get merged correctly if they're complex objects in turn. Improved the exception message for invalid payloads for JSON Patch requests to indicate the expected payload as the root Jackson exception does not contain any hints to what's actually expected. --- .../rest/webmvc/config/JsonPatchHandler.java | 30 +++++++++-- ...ResourceHandlerMethodArgumentResolver.java | 11 +++- .../rest/webmvc/json/DomainObjectReader.java | 51 ++++++++++++++++++- .../config/JsonPatchHandlerUnitTests.java | 17 +++++++ .../json/DomainObjectReaderUnitTests.java | 21 ++++++++ 5 files changed, 121 insertions(+), 9 deletions(-) diff --git a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/config/JsonPatchHandler.java b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/config/JsonPatchHandler.java index 9182abeae..8b847eea8 100644 --- a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/config/JsonPatchHandler.java +++ b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/config/JsonPatchHandler.java @@ -20,7 +20,9 @@ import java.lang.reflect.Field; import java.util.List; import org.springframework.data.rest.webmvc.IncomingRequest; +import org.springframework.data.rest.webmvc.RestMediaTypes; import org.springframework.data.rest.webmvc.json.DomainObjectReader; +import org.springframework.http.converter.HttpMessageNotReadableException; import org.springframework.util.Assert; import org.springframework.util.ReflectionUtils; @@ -102,9 +104,7 @@ class JsonPatchHandler { T applyPatch(InputStream source, T target) throws Exception { - CollectionType listOfOperationsType = mapper.getTypeFactory().constructCollectionType(List.class, - JsonPatchOperation.class); - List readValue = mapper.readValue(source, listOfOperationsType); + List readValue = getPatchOperations(source); JsonNode existingAsNode = mapper.readTree(sourceMapper.writeValueAsBytes(target)); JsonNode patchedNode = existingAsNode; @@ -115,8 +115,8 @@ class JsonPatchHandler { // Replace remove operation with replace operation and a value of null. JsonPointer path = (JsonPointer) ReflectionUtils.getField(PATH_FIELD, operation); - patchedNode = isCollectionElementReference(path) ? operation.apply(patchedNode) : new ReplaceOperation(path, - NullNode.getInstance()).apply(patchedNode); + patchedNode = isCollectionElementReference(path) ? operation.apply(patchedNode) + : new ReplaceOperation(path, NullNode.getInstance()).apply(patchedNode); } else { patchedNode = operation.apply(patchedNode); @@ -134,6 +134,26 @@ class JsonPatchHandler { return reader.readPut(source, existingObject, mapper); } + /** + * Returns all {@link JsonPatchOperation}s to be applied. + * + * @param source must not be {@literal null}. + * @return + * @throws HttpMessageNotReadableException in case the payload can't be read. + */ + private List getPatchOperations(InputStream source) { + + CollectionType listOfOperationsType = mapper.getTypeFactory().constructCollectionType(List.class, + JsonPatchOperation.class); + + try { + return mapper.readValue(source, listOfOperationsType); + } catch (Exception o_O) { + throw new HttpMessageNotReadableException( + String.format("Could not read PATCH operations! Expected %s!", RestMediaTypes.JSON_PATCH_JSON), o_O); + } + } + /** * Returns whether the trailing element of the given {@link JsonPointer} is a pointer into an array or collection. * diff --git a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/config/PersistentEntityResourceHandlerMethodArgumentResolver.java b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/config/PersistentEntityResourceHandlerMethodArgumentResolver.java index 6ffb8156b..8605e709d 100644 --- a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/config/PersistentEntityResourceHandlerMethodArgumentResolver.java +++ b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/config/PersistentEntityResourceHandlerMethodArgumentResolver.java @@ -166,8 +166,8 @@ public class PersistentEntityResourceHandlerMethodArgumentResolver implements Ha ObjectMapper mapper = ((MappingJackson2HttpMessageConverter) converter).getObjectMapper(); - return objectToUpdate == null ? read(request, converter, information) : readPutForUpdate(request, mapper, - objectToUpdate); + return objectToUpdate == null ? read(request, converter, information) + : readPutForUpdate(request, mapper, objectToUpdate); } // Catch all @@ -177,9 +177,16 @@ public class PersistentEntityResourceHandlerMethodArgumentResolver implements Ha private Object readPatch(IncomingRequest request, ObjectMapper mapper, Object existingObject) { try { + JsonPatchHandler handler = new JsonPatchHandler(mapper, reader); return handler.apply(request, existingObject); + } catch (Exception o_O) { + + if (o_O instanceof HttpMessageNotReadableException) { + throw (HttpMessageNotReadableException) o_O; + } + throw new HttpMessageNotReadableException(String.format(ERROR_MESSAGE, existingObject.getClass()), o_O); } } 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 ba3eec964..44b1b1bde 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 @@ -184,16 +184,63 @@ public class DomainObjectReader { PersistentPropertyAccessor accessor = entity.getPropertyAccessor(target); Object nested = accessor.getProperty(property); - if (nested != null && property.isEntity()) { - doMerge((ObjectNode) child, nested, mapper); + ObjectNode objectNode = (ObjectNode) child; + + if (property.isMap()) { + + // Keep empty Map to wipe it as expected + if (!objectNode.fieldNames().hasNext()) { + continue; + } + + doMergeNestedMap((Map) nested, objectNode, mapper); + + // Remove potentially emptied Map as values have been handled recursively + if (!objectNode.fieldNames().hasNext()) { + i.remove(); + } + + continue; } + if (nested != null && property.isEntity()) { + doMerge(objectNode, nested, mapper); + } } } return mapper.readerForUpdating(target).readValue(root); } + /** + * Merges nested {@link Map} values for the given source {@link Map}, the {@link ObjectNode} and {@link ObjectMapper}. + * + * @param source can be {@literal null}. + * @param node must not be {@literal null}. + * @param mapper must not be {@literal null}. + * @throws Exception + */ + private void doMergeNestedMap(Map source, ObjectNode node, ObjectMapper mapper) throws Exception { + + if (source == null) { + return; + } + + Iterator> fields = node.fields(); + + while (fields.hasNext()) { + + Entry entry = fields.next(); + JsonNode child = entry.getValue(); + Object sourceValue = source.get(entry.getKey()); + + if (child instanceof ObjectNode && sourceValue != null) { + doMerge((ObjectNode) child, sourceValue, mapper); + fields.remove(); + } + } + } + /** * Returns the {@link MappedProperties} for the given {@link PersistentEntity}. * diff --git a/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/config/JsonPatchHandlerUnitTests.java b/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/config/JsonPatchHandlerUnitTests.java index b6ebf7f0d..0fab114e8 100644 --- a/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/config/JsonPatchHandlerUnitTests.java +++ b/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/config/JsonPatchHandlerUnitTests.java @@ -22,16 +22,20 @@ import static org.springframework.data.rest.webmvc.util.TestUtils.*; import java.util.Arrays; import org.junit.Before; +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.ExpectedException; import org.junit.runner.RunWith; import org.mockito.Mock; import org.mockito.runners.MockitoJUnitRunner; import org.springframework.data.mapping.context.PersistentEntities; import org.springframework.data.mongodb.core.mapping.MongoMappingContext; import org.springframework.data.rest.core.mapping.ResourceMappings; +import org.springframework.data.rest.webmvc.RestMediaTypes; import org.springframework.data.rest.webmvc.json.DomainObjectReader; import org.springframework.data.rest.webmvc.mongodb.Address; import org.springframework.data.rest.webmvc.mongodb.User; +import org.springframework.http.converter.HttpMessageNotReadableException; import com.fasterxml.jackson.databind.ObjectMapper; @@ -47,6 +51,7 @@ public class JsonPatchHandlerUnitTests { User user; @Mock ResourceMappings mappings; + public @Rule ExpectedException exception = ExpectedException.none(); @Before public void setUp() { @@ -118,4 +123,16 @@ public class JsonPatchHandlerUnitTests { assertThat(user.colleagues, hasSize(1)); assertThat(user.colleagues.get(0).firstname, is(christoph.firstname)); } + + /** + * @see DATAREST-609 + */ + @Test + public void hintsToMediaTypeIfBodyCantBeRead() throws Exception { + + exception.expect(HttpMessageNotReadableException.class); + exception.expectMessage(RestMediaTypes.JSON_PATCH_JSON.toString()); + + handler.applyPatch(asStream("{ \"foo\" : \"bar\" }"), new User()); + } } 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 b2c0ddf4d..a66c129a7 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,6 +19,7 @@ import static org.hamcrest.CoreMatchers.*; import static org.junit.Assert.*; import java.util.Collections; +import java.util.Map; import org.junit.Before; import org.junit.Test; @@ -94,11 +95,31 @@ public class DomainObjectReaderUnitTests { assertThat(result.lastName, is("Beauford")); } + /** + * @see DATAREST-605 + */ + @Test + public void mergesMapCorrectly() throws Exception { + + SampleUser user = new SampleUser("firstname", "password"); + user.relatedUsers = Collections.singletonMap("parent", new SampleUser("firstname", "password")); + + JsonNode node = new ObjectMapper() + .readTree("{ \"relatedUsers\" : { \"parent\" : { \"password\" : \"sneeky\", \"name\" : \"Oliver\" } } }"); + + SampleUser result = reader.readPut((ObjectNode) node, user, new ObjectMapper()); + + // Assert that the nested Map values also consider ignored properties + assertThat(result.relatedUsers.get("parent").password, is("password")); + assertThat(result.relatedUsers.get("parent").name, is("Oliver")); + } + @JsonAutoDetect(fieldVisibility = Visibility.ANY) static class SampleUser { String name; @JsonIgnore String password; + Map relatedUsers; public SampleUser(String name, String password) { this.name = name;