From 0813faf0d505c48da4dc46074ef634b1bdacc838 Mon Sep 17 00:00:00 2001 From: Oliver Gierke Date: Fri, 12 May 2017 14:32:48 +0200 Subject: [PATCH] DATAREST-1039 - Fix appending a complex value appended to a collection via JSON Patch. When a PATCH call using JSON Patch tried to append an item to an empty collection, it previously failed to look up the type to unmarshal the incoming payload to. We now inspect the declared types on the wrapped object's property to determine that type for append operations. Also, an uninitialized collection is now initialized using Spring's CollectionFactory. --- .../rest/webmvc/json/patch/AddOperation.java | 31 +++++++++++++++++++ .../webmvc/json/patch/PatchOperation.java | 19 +++++++++++- .../rest/webmvc/json/patch/PathToSpEL.java | 7 ++++- .../webmvc/json/patch/AddOperationTests.java | 28 +++++++++++++++++ .../data/rest/webmvc/json/patch/Todo.java | 7 +++-- 5 files changed, 87 insertions(+), 5 deletions(-) diff --git a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/AddOperation.java b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/AddOperation.java index 4d0ca6827..124941117 100644 --- a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/AddOperation.java +++ b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/AddOperation.java @@ -15,6 +15,12 @@ */ package org.springframework.data.rest.webmvc.json.patch; +import java.util.ArrayList; +import java.util.List; + +import org.springframework.data.mapping.PropertyPath; +import org.springframework.util.StringUtils; + /** * Operation to add a new value to the given "path". Will throw a {@link PatchException} if the path is invalid or if * the given value is not assignable to the given path. @@ -42,4 +48,29 @@ class AddOperation extends PatchOperation { void perform(Object targetObject, Class type) { addValue(targetObject, evaluateValueFromTarget(targetObject, type)); } + + /* + * (non-Javadoc) + * @see org.springframework.data.rest.webmvc.json.patch.PatchOperation#evaluateValueFromTarget(java.lang.Object, java.lang.Class) + */ + @Override + protected Object evaluateValueFromTarget(Object targetObject, Class entityType) { + + if (!path.endsWith("-")) { + return super.evaluateValueFromTarget(targetObject, entityType); + } + + List segments = new ArrayList(); + + for (String segment : path.split("/")) { + if (!(segment.matches("\\d+") || segment.equals("-") || segment.isEmpty())) { + segments.add(segment); + } + } + + PropertyPath propertyPath = PropertyPath.from(StringUtils.collectionToDelimitedString(segments, "."), entityType); + + return value instanceof LateObjectEvaluator ? ((LateObjectEvaluator) value).evaluate(propertyPath.getType()) + : value; + } } diff --git a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/PatchOperation.java b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/PatchOperation.java index f8b9ce3dc..cd8dbe5b7 100644 --- a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/PatchOperation.java +++ b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/PatchOperation.java @@ -17,8 +17,11 @@ package org.springframework.data.rest.webmvc.json.patch; import static org.springframework.data.rest.webmvc.json.patch.PathToSpEL.*; +import java.util.Collection; import java.util.List; +import org.springframework.core.CollectionFactory; +import org.springframework.core.convert.TypeDescriptor; import org.springframework.expression.Expression; import org.springframework.expression.ExpressionException; @@ -129,7 +132,21 @@ public abstract class PatchOperation { Integer listIndex = targetListIndex(path); if (parent == null || !(parent instanceof List) || listIndex == null) { - spelExpression.setValue(target, value); + + TypeDescriptor descriptor = parentExpression.getValueTypeDescriptor(target); + + // Set as new collection if necessary + if (descriptor.isCollection() && !Collection.class.isInstance(value)) { + + Collection collection = CollectionFactory.createCollection(descriptor.getType(), 1); + collection.add(value); + + parentExpression.setValue(target, collection); + + } else { + spelExpression.setValue(target, value); + } + } else { List list = (List) parentExpression.getValue(target); diff --git a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/PathToSpEL.java b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/PathToSpEL.java index 2a231cda3..ce17d0306 100644 --- a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/PathToSpEL.java +++ b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/PathToSpEL.java @@ -79,7 +79,12 @@ public class PathToSpEL { } if (APPEND_CHARACTERS.contains(pathNode)) { - spelBuilder.append("[size() - 1]"); + + if (spelBuilder.length() > 0) { + spelBuilder.append("."); + } + + spelBuilder.append("$[true]"); continue; } diff --git a/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/AddOperationTests.java b/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/AddOperationTests.java index bd74f7e12..f6c655a21 100644 --- a/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/AddOperationTests.java +++ b/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/AddOperationTests.java @@ -23,6 +23,9 @@ import java.util.List; import org.junit.Test; +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; + public class AddOperationTests { @Test @@ -84,4 +87,29 @@ public class AddOperationTests { assertThat(todo.getItems().get(0), is("Some text.")); } + + @Test // DATAREST-1039 + public void addsLazilyEvaluatedObjectToList() throws Exception { + + Todo todo = new Todo(1L, "description", false); + + ObjectMapper mapper = new ObjectMapper(); + JsonNode node = mapper.readTree("\"Some text.\""); + JsonLateObjectEvaluator evaluator = new JsonLateObjectEvaluator(mapper, node); + + new AddOperation("/items/-", evaluator).perform(todo, Todo.class); + + assertThat(todo.getItems().get(0), is("Some text.")); + } + + @Test // DATAREST-1039 + public void initializesNullCollectionsOnAppend() { + + Todo todo = new Todo(1L, "description", false); + + new AddOperation("/uninitialized/-", "Text").perform(todo, Todo.class); + + assertThat(todo.getUninitialized(), is(notNullValue())); + assertThat(todo.getUninitialized(), hasItem("Text")); + } } diff --git a/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/Todo.java b/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/Todo.java index 5046ea84c..2405f144f 100644 --- a/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/Todo.java +++ b/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/Todo.java @@ -16,13 +16,13 @@ package org.springframework.data.rest.webmvc.json.patch; +import lombok.Data; +import lombok.NoArgsConstructor; + import java.math.BigInteger; import java.util.ArrayList; import java.util.List; -import lombok.Data; -import lombok.NoArgsConstructor; - /** * @author Roy Clarkson * @author Craig Walls @@ -38,6 +38,7 @@ class Todo { private boolean complete; private TodoType type = new TodoType(); private List items = new ArrayList(); + private List uninitialized; private BigInteger amount; public Todo(Long id, String description, boolean complete) {