From 9a9512767efb013471887eda5d652148f78c20a3 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 | 28 +++++++++++++++++++ .../webmvc/json/patch/PatchOperation.java | 19 ++++++++++++- .../rest/webmvc/json/patch/PathToSpEL.java | 7 ++++- .../webmvc/json/patch/AddOperationTests.java | 27 ++++++++++++++++++ .../data/rest/webmvc/json/patch/Todo.java | 1 + 5 files changed, 80 insertions(+), 2 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..36fa09a14 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,11 @@ */ package org.springframework.data.rest.webmvc.json.patch; +import java.util.Arrays; +import java.util.stream.Collectors; + +import org.springframework.data.mapping.PropertyPath; + /** * 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 +47,27 @@ 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); + } + + String pathSource = Arrays.stream(path.split("/"))// + .filter(it -> !it.matches("\\d")) // no digits + .filter(it -> !it.equals("-")) // no "last element"s + .filter(it -> !it.isEmpty()) // + .collect(Collectors.joining(".")); + + PropertyPath propertyPath = PropertyPath.from(pathSource, 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 d688a4b03..e8a20d648 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 0c42a7d14..bbb0fe3e4 100755 --- 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,28 @@ public class AddOperationTests { assertThat(todo.getItems().get(0)).isEqualTo("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)).isEqualTo("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()).containsExactly("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 4d937fbd3..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 @@ -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) {