Spring/spring-data-rest
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

DATAREST-1152 - Overhaul of patch expression handling.

Browse Source 
Significantly refactored the way that patch path expressions are handled and evaluated. The new design is centered around SpelPath that is aware of the original path as well as the derived SpEL expression. That SpelPath then requires clients to bind it to a type so that the original path can be validated (and rejected if invalid) and provide API to read, set, copy and move values backed by the original path. Both SpelPath and TypedSpelPath instances are cached to avoid repeated creation.

PatchOperation implementations now provide more fluent factory methods, in some cases via intermediate builders. Removed a lot of obsolete code that created JsonNodes from a list of PatchOperations as we don't actually use that functionality anywhere. Removed obsolete generics where possible.
This commit is contained in:
Oliver Gierke
2017-10-25 17:04:16 +02:00
parent 74e12a55b7
commit d48499ad8d
23 changed files with 671 additions and 522 deletions
Download Patch File Download Diff File Expand all files Collapse all files

14
spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/AddOperationTests.java → spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/AddOperationUnitTests.java
View File

@@ -26,7 +26,7 @@ import org.junit.Test;
import com.fasterxml.jackson.databind.JsonNode;
import com.fasterxml.jackson.databind.ObjectMapper;
public class AddOperationTests {
public class AddOperationUnitTests {
@Test
public void addBooleanPropertyValue() throws Exception {
@@ -36,7 +36,7 @@ public class AddOperationTests {
todos.add(new Todo(2L, "B", false));
todos.add(new Todo(3L, "C", false));
AddOperation add = new AddOperation("/1/complete", true);
AddOperation add = AddOperation.of("/1/complete", true);
add.perform(todos, Todo.class);
assertTrue(todos.get(1).isComplete());
@@ -50,7 +50,7 @@ public class AddOperationTests {
todos.add(new Todo(2L, "B", false));
todos.add(new Todo(3L, "C", false));
AddOperation add = new AddOperation("/1/description", "BBB");
AddOperation add = AddOperation.of("/1/description", "BBB");
add.perform(todos, Todo.class);
assertEquals("BBB", todos.get(1).getDescription());
@@ -64,7 +64,7 @@ public class AddOperationTests {
todos.add(new Todo(2L, "B", false));
todos.add(new Todo(3L, "C", false));
AddOperation add = new AddOperation("/1", new Todo(null, "D", true));
AddOperation add = AddOperation.of("/1", new Todo(null, "D", true));
add.perform(todos, Todo.class);
assertEquals(4, todos.size());
@@ -83,7 +83,7 @@ public class AddOperationTests {
Todo todo = new Todo(1L, "description", false);
new AddOperation("/items/-", "Some text.").perform(todo, Todo.class);
AddOperation.of("/items/-", "Some text.").perform(todo, Todo.class);
assertThat(todo.getItems().get(0)).isEqualTo("Some text.");
}
@@ -97,7 +97,7 @@ public class AddOperationTests {
JsonNode node = mapper.readTree("\"Some text.\"");
JsonLateObjectEvaluator evaluator = new JsonLateObjectEvaluator(mapper, node);
new AddOperation("/items/-", evaluator).perform(todo, Todo.class);
AddOperation.of("/items/-", evaluator).perform(todo, Todo.class);
assertThat(todo.getItems().get(0)).isEqualTo("Some text.");
}
@@ -107,7 +107,7 @@ public class AddOperationTests {
Todo todo = new Todo(1L, "description", false);
new AddOperation("/uninitialized/-", "Text").perform(todo, Todo.class);
AddOperation.of("/uninitialized/-", "Text").perform(todo, Todo.class);
assertThat(todo.getUninitialized()).containsExactly("Text");
}

18
spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/CopyOperationTests.java → spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/CopyOperationUnitTests.java
View File

@@ -22,7 +22,7 @@ import java.util.List;
import org.junit.Test;
public class CopyOperationTests {
public class CopyOperationUnitTests {
@Test
public void copyBooleanPropertyValue() throws Exception {
@@ -32,7 +32,7 @@ public class CopyOperationTests {
todos.add(new Todo(2L, "B", false));
todos.add(new Todo(3L, "C", false));
CopyOperation copy = new CopyOperation("/1/complete", "/0/complete");
CopyOperation copy = CopyOperation.from("/0/complete").to("/1/complete");
copy.perform(todos, Todo.class);
assertTrue(todos.get(1).isComplete());
@@ -46,7 +46,7 @@ public class CopyOperationTests {
todos.add(new Todo(2L, "B", false));
todos.add(new Todo(3L, "C", false));
CopyOperation copy = new CopyOperation("/1/description", "/0/description");
CopyOperation copy = CopyOperation.from("/0/description").to("/1/description");
copy.perform(todos, Todo.class);
assertEquals("A", todos.get(1).getDescription());
@@ -60,7 +60,7 @@ public class CopyOperationTests {
todos.add(new Todo(2L, "B", false));
todos.add(new Todo(3L, "C", false));
CopyOperation copy = new CopyOperation("/1/description", "/0/complete");
CopyOperation copy = CopyOperation.from("/0/complete").to("/1/description");
copy.perform(todos, Todo.class);
assertEquals("true", todos.get(1).getDescription());
@@ -74,7 +74,7 @@ public class CopyOperationTests {
todos.add(new Todo(2L, "B", true));
todos.add(new Todo(3L, "C", false));
CopyOperation copy = new CopyOperation("/0", "/1");
CopyOperation copy = CopyOperation.from("/1").to("/0");
copy.perform(todos, Todo.class);
assertEquals(4, todos.size());
@@ -92,7 +92,7 @@ public class CopyOperationTests {
todos.add(new Todo(2L, "B", false));
todos.add(new Todo(3L, "C", false));
CopyOperation copy = new CopyOperation("/2", "/0");
CopyOperation copy = CopyOperation.from("/0").to("/2");
copy.perform(todos, Todo.class);
assertEquals(4, todos.size());
@@ -110,7 +110,7 @@ public class CopyOperationTests {
todos.add(new Todo(2L, "B", false));
todos.add(new Todo(3L, "C", false));
CopyOperation copy = new CopyOperation("/3", "/0");
CopyOperation copy = CopyOperation.from("/0").to("/3");
copy.perform(todos, Todo.class);
assertEquals(4, todos.size());
@@ -128,7 +128,7 @@ public class CopyOperationTests {
todos.add(new Todo(2L, "B", false));
todos.add(new Todo(3L, "C", false));
CopyOperation copy = new CopyOperation("/-", "/0");
CopyOperation copy = CopyOperation.from("/0").to("/-");
copy.perform(todos, Todo.class);
assertEquals(4, todos.size());
@@ -144,7 +144,7 @@ public class CopyOperationTests {
todos.add(new Todo(2L, "B", false));
todos.add(new Todo(3L, "C", false));
CopyOperation copy = new CopyOperation("/0", "/-");
CopyOperation copy = CopyOperation.from("/-").to("/0");
copy.perform(todos, Todo.class);
assertEquals(4, todos.size());

2
spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/JsonPatchTests.java → spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/JsonPatchUnitTests.java
View File

@@ -43,7 +43,7 @@ import com.fasterxml.jackson.databind.ObjectMapper;
* @author Mathias Düsterhöft
* @author Oliver Trosien
*/
public class JsonPatchTests {
public class JsonPatchUnitTests {
public @Rule ExpectedException exception = ExpectedException.none();

20
spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/MoveOperationTests.java → spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/MoveOperationUnitTests.java
View File

@@ -22,7 +22,7 @@ import java.util.List;
import org.junit.Test;
public class MoveOperationTests {
public class MoveOperationUnitTests {
@Test
public void moveBooleanPropertyValue() throws Exception {
@@ -33,14 +33,14 @@ public class MoveOperationTests {
todos.add(new Todo(3L, "C", false));
try {
MoveOperation move = new MoveOperation("/1/complete", "/0/complete");
MoveOperation move = MoveOperation.from("/0/complete").to("/1/complete");
move.perform(todos, Todo.class);
fail();
} catch (PatchException e) {
assertEquals("Path '/0/complete' is not nullable.", e.getMessage());
}
assertFalse(todos.get(1).isComplete());
assertFalse(todos.get(1).isComplete());
}
@Test
@@ -51,7 +51,7 @@ public class MoveOperationTests {
todos.add(new Todo(2L, "B", false));
todos.add(new Todo(3L, "C", false));
MoveOperation move = new MoveOperation("/1/description", "/0/description");
MoveOperation move = MoveOperation.from("/0/description").to("/1/description");
move.perform(todos, Todo.class);
assertEquals("A", todos.get(1).getDescription());
@@ -66,7 +66,7 @@ public class MoveOperationTests {
todos.add(new Todo(3L, "C", false));
try {
MoveOperation move = new MoveOperation("/1/description", "/0/complete");
MoveOperation move = MoveOperation.from("/0/complete").to("/1/description");
move.perform(todos, Todo.class);
fail();
} catch (PatchException e) {
@@ -91,7 +91,7 @@ public class MoveOperationTests {
todos.add(new Todo(2L, "B", true));
todos.add(new Todo(3L, "C", false));
MoveOperation move = new MoveOperation("/0", "/1");
MoveOperation move = MoveOperation.from("/1").to("/0");
move.perform(todos, Todo.class);
assertEquals(3, todos.size());
@@ -108,7 +108,7 @@ public class MoveOperationTests {
todos.add(new Todo(2L, "B", false));
todos.add(new Todo(3L, "C", false));
MoveOperation move = new MoveOperation("/2", "/0");
MoveOperation move = MoveOperation.from("/0").to("/2");
move.perform(todos, Todo.class);
assertEquals(3, todos.size());
@@ -125,7 +125,7 @@ public class MoveOperationTests {
todos.add(new Todo(2L, "B", false));
todos.add(new Todo(3L, "C", false));
MoveOperation move = new MoveOperation("/2", "/0");
MoveOperation move = MoveOperation.from("/0").to("/2");
move.perform(todos, Todo.class);
assertEquals(3, todos.size());
@@ -149,7 +149,7 @@ public class MoveOperationTests {
expected.add(new Todo(3L, "C", false));
expected.add(new Todo(4L, "E", false));
MoveOperation move = new MoveOperation("/1", "/-");
MoveOperation move = MoveOperation.from("/-").to("/1");
move.perform(todos, Todo.class);
assertEquals(expected, todos);
}
@@ -169,7 +169,7 @@ public class MoveOperationTests {
expected.add(new Todo(4L, "E", false));
expected.add(new Todo(2L, "G", false));
MoveOperation move = new MoveOperation("/-", "/1");
MoveOperation move = MoveOperation.from("/1").to("/-");
move.perform(todos, Todo.class);
assertEquals(expected, todos);
}

42
spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/PatchOperationUnitTests.java
View File

@@ -17,30 +17,48 @@ package org.springframework.data.rest.webmvc.json.patch;
import static org.assertj.core.api.Assertions.*;
import java.util.Arrays;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
import org.junit.runners.Parameterized.Parameter;
import org.junit.runners.Parameterized.Parameters;
/**
* General unit tests for {@link PatchOperation} implementations.
*
* @author Oliver Gierke
*/
@RunWith(Parameterized.class)
public class PatchOperationUnitTests {
@Parameters
public static Iterable<? extends PatchOperation> operations() {
String invalidPath = "/nonExistant";
String validPath = "/1/description";
return Arrays.asList( //
AddOperation.of(invalidPath, null), //
RemoveOperation.valueAt(invalidPath), //
ReplaceOperation.valueAt(invalidPath).with(null), //
TestOperation.whetherValueAt(invalidPath).hasValue(null), //
CopyOperation.from(invalidPath).to(validPath), //
CopyOperation.from(validPath).to(invalidPath), //
MoveOperation.from(invalidPath).to(validPath), //
MoveOperation.from(validPath).to(invalidPath) //
);
}
public @Parameter(0) PatchOperation operation;
@Test // DATAREST-1137
public void invalidPathGetsRejected() {
String invalidPath = "/nonExistant";
verifyIllegalPath(new AddOperation(invalidPath, null));
verifyIllegalPath(new CopyOperation(invalidPath, null));
verifyIllegalPath(new MoveOperation(invalidPath, null));
verifyIllegalPath(new RemoveOperation(invalidPath));
verifyIllegalPath(new ReplaceOperation(invalidPath, null));
verifyIllegalPath(new TestOperation(invalidPath, null));
}
private static void verifyIllegalPath(PatchOperation operation) {
Todo todo = new Todo(1L, "A", false);
assertThatExceptionOfType(PatchException.class) //

4
spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/RemoveOperationTests.java
View File

@@ -32,7 +32,7 @@ public class RemoveOperationTests {
todos.add(new Todo(2L, "B", false));
todos.add(new Todo(3L, "C", false));
new RemoveOperation("/1/description").perform(todos, Todo.class);
RemoveOperation.valueAt("/1/description").perform(todos, Todo.class);
assertNull(todos.get(1).getDescription());
}
@@ -45,7 +45,7 @@ public class RemoveOperationTests {
todos.add(new Todo(2L, "B", false));
todos.add(new Todo(3L, "C", false));
new RemoveOperation("/1").perform(todos, Todo.class);
RemoveOperation.valueAt("/1").perform(todos, Todo.class);
assertEquals(2, todos.size());
assertEquals("A", todos.get(0).getDescription());

10
spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/ReplaceOperationTests.java
View File

@@ -34,7 +34,7 @@ public class ReplaceOperationTests {
todos.add(new Todo(2L, "B", false));
todos.add(new Todo(3L, "C", false));
ReplaceOperation replace = new ReplaceOperation("/1/complete", true);
ReplaceOperation replace = ReplaceOperation.valueAt("/1/complete").with(true);
replace.perform(todos, Todo.class);
assertTrue(todos.get(1).isComplete());
@@ -48,7 +48,7 @@ public class ReplaceOperationTests {
todos.add(new Todo(2L, "B", false));
todos.add(new Todo(3L, "C", false));
ReplaceOperation replace = new ReplaceOperation("/1/description", "BBB");
ReplaceOperation replace = ReplaceOperation.valueAt("/1/description").with("BBB");
replace.perform(todos, Todo.class);
assertEquals("BBB", todos.get(1).getDescription());
@@ -62,7 +62,7 @@ public class ReplaceOperationTests {
todos.add(new Todo(2L, "B", false));
todos.add(new Todo(3L, "C", false));
ReplaceOperation replace = new ReplaceOperation("/1/description", 22);
ReplaceOperation replace = ReplaceOperation.valueAt("/1/description").with(22);
replace.perform(todos, Todo.class);
assertEquals("22", todos.get(1).getDescription());
@@ -74,8 +74,8 @@ public class ReplaceOperationTests {
Todo todo = new Todo(1L, "A", false);
ObjectMapper mapper = new ObjectMapper();
ReplaceOperation replace = new ReplaceOperation("/type",
new JsonLateObjectEvaluator(mapper, mapper.readTree("{ \"value\" : \"new\" }")));
ReplaceOperation replace = ReplaceOperation.valueAt("/type")
.with(new JsonLateObjectEvaluator(mapper, mapper.readTree("{ \"value\" : \"new\" }")));
replace.perform(todo, Todo.class);
assertNotNull(todo.getType());

31
spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/PathToSpelTests.java → spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/SpelPathUnitTests.java
View File

@@ -21,33 +21,52 @@ import java.util.ArrayList;
import java.util.List;
import org.junit.Test;
import org.springframework.expression.Expression;
import org.springframework.data.rest.webmvc.json.patch.SpelPath.TypedSpelPath;
public class PathToSpelTests {
public class SpelPathUnitTests {
@Test
public void listIndex() {
Expression expr = PathToSpEL.pathToExpression("/1/description");
SpelPath expr = SpelPath.of("/1/description");
List<Todo> todos = new ArrayList<Todo>();
todos.add(new Todo(1L, "A", false));
todos.add(new Todo(2L, "B", false));
todos.add(new Todo(3L, "C", false));
assertEquals("B", (String) expr.getValue(todos));
assertEquals("B", (String) expr.bindTo(Todo.class).getValue(todos));
}
@Test
public void accessesLastCollectionElementWithDash() {
Expression expr = PathToSpEL.pathToExpression("/-/description");
SpelPath expr = SpelPath.of("/-/description");
List<Todo> todos = new ArrayList<Todo>();
todos.add(new Todo(1L, "A", false));
todos.add(new Todo(2L, "B", false));
todos.add(new Todo(3L, "C", false));
assertEquals("C", (String) expr.getValue(todos));
assertEquals("C", (String) expr.bindTo(Todo.class).getValue(todos));
}
@Test // DATAREST-1152
public void cachesSpelPath() {
SpelPath left = SpelPath.of("/description");
SpelPath right = SpelPath.of("/description");
assertSame(left, right);
}
@Test // DATAREST-1152
public void cachesTypedSpelPath() {
SpelPath source = SpelPath.of("/description");
TypedSpelPath left = source.bindTo(Todo.class);
TypedSpelPath right = source.bindTo(Todo.class);
assertSame(left, right);
}
}

10
spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/TestOperationTests.java → spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/TestOperationUnitTests.java
View File

@@ -20,7 +20,7 @@ import java.util.List;
import org.junit.Test;
public class TestOperationTests {
public class TestOperationUnitTests {
@Test
public void testPropertyValueEquals() throws Exception {
@@ -30,10 +30,10 @@ public class TestOperationTests {
todos.add(new Todo(2L, "B", true));
todos.add(new Todo(3L, "C", false));
TestOperation test = new TestOperation("/0/complete", false);
TestOperation test = TestOperation.whetherValueAt("/0/complete").hasValue(false);
test.perform(todos, Todo.class);
TestOperation test2 = new TestOperation("/1/complete", true);
TestOperation test2 = TestOperation.whetherValueAt("/1/complete").hasValue(true);
test2.perform(todos, Todo.class);
}
@@ -46,7 +46,7 @@ public class TestOperationTests {
todos.add(new Todo(2L, "B", true));
todos.add(new Todo(3L, "C", false));
TestOperation test = new TestOperation("/0/complete", true);
TestOperation test = TestOperation.whetherValueAt("/0/complete").hasValue(true);
test.perform(todos, Todo.class);
}
@@ -58,7 +58,7 @@ public class TestOperationTests {
todos.add(new Todo(2L, "B", true));
todos.add(new Todo(3L, "C", false));
TestOperation test = new TestOperation("/1", new Todo(2L, "B", true));
TestOperation test = TestOperation.whetherValueAt("/1").hasValue(new Todo(2L, "B", true));
test.perform(todos, Todo.class);
}
}