From 973f932a390ec1dd6e4b8e78f75b355238db12a8 Mon Sep 17 00:00:00 2001 From: Oliver Gierke Date: Mon, 25 Sep 2017 18:57:15 +0200 Subject: [PATCH] DATAREST-1137 - All patch operations now verify path expressions. We now make sure that all patch operations now get the path they're supposed to be applied to verified before execution. --- .../rest/webmvc/json/patch/AddOperation.java | 6 +-- .../rest/webmvc/json/patch/CopyOperation.java | 6 +-- .../rest/webmvc/json/patch/MoveOperation.java | 6 +-- .../webmvc/json/patch/PatchOperation.java | 20 ++++++-- .../webmvc/json/patch/RemoveOperation.java | 8 +-- .../webmvc/json/patch/ReplaceOperation.java | 9 ++-- .../rest/webmvc/json/patch/TestOperation.java | 9 ++-- .../webmvc/json/patch/JsonPatchTests.java | 9 +++- .../json/patch/PatchOperationUnitTests.java | 49 +++++++++++++++++++ 9 files changed, 96 insertions(+), 26 deletions(-) create mode 100644 spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/PatchOperationUnitTests.java 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 a303125b3..11c79d9b2 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 @@ -1,5 +1,5 @@ /* - * Copyright 2014-2016 the original author or authors. + * Copyright 2014-2017 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -36,10 +36,10 @@ class AddOperation extends PatchOperation { /* * (non-Javadoc) - * @see org.springframework.data.rest.webmvc.json.patch.PatchOperation#perform(java.lang.Object, java.lang.Class) + * @see org.springframework.data.rest.webmvc.json.patch.PatchOperation#doPerform(java.lang.Object, java.lang.Class) */ @Override - void perform(Object targetObject, Class type) { + void doPerform(Object targetObject, Class type) { addValue(targetObject, evaluateValueFromTarget(targetObject, type)); } diff --git a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/CopyOperation.java b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/CopyOperation.java index ee8f84817..f432e59ae 100644 --- a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/CopyOperation.java +++ b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/CopyOperation.java @@ -1,5 +1,5 @@ /* - * Copyright 2014-2016 the original author or authors. + * Copyright 2014-2017 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -53,10 +53,10 @@ class CopyOperation extends FromOperation { /* * (non-Javadoc) - * @see org.springframework.data.rest.webmvc.json.patch.PatchOperation#perform(java.lang.Object, java.lang.Class) + * @see org.springframework.data.rest.webmvc.json.patch.PatchOperation#doPerform(java.lang.Object, java.lang.Class) */ @Override - void perform(Object target, Class type) { + void doPerform(Object target, Class type) { addValue(target, pathToExpression(getFrom()).getValue(target)); } } diff --git a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/MoveOperation.java b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/MoveOperation.java index ef1869c4a..3359fd3a4 100644 --- a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/MoveOperation.java +++ b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/MoveOperation.java @@ -1,5 +1,5 @@ /* - * Copyright 2014-2016 the original author or authors. + * Copyright 2014-2017 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -44,10 +44,10 @@ class MoveOperation extends FromOperation { /* * (non-Javadoc) - * @see org.springframework.data.rest.webmvc.json.patch.PatchOperation#perform(java.lang.Object, java.lang.Class) + * @see org.springframework.data.rest.webmvc.json.patch.PatchOperation#doPerform(java.lang.Object, java.lang.Class) */ @Override - void perform(Object target, Class type) { + void doPerform(Object target, Class type) { addValue(target, popValueAtPath(target, getFrom())); } } 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 811734faa..bbad0b199 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 @@ -234,11 +234,25 @@ public abstract class PatchOperation { } /** - * Perform the operation. + * Perform the operation in the given target object. * - * @param target the target of the operation. + * @param target the target of the operation, must not be {@literal null}. + * @param type must not be {@literal null}. */ - abstract void perform(Object target, Class type); + final void perform(Object target, Class type) { + + verifyPath(type); + + doPerform(target, type); + } + + /** + * Implements the actually application of the operation. + * + * @param target must not be {@literal null}. + * @param type must not be {@literal null}. + */ + abstract void doPerform(Object target, Class type); private Integer targetListIndex(String path) { diff --git a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/RemoveOperation.java b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/RemoveOperation.java index 51f638ad3..90a3bf98a 100644 --- a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/RemoveOperation.java +++ b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/RemoveOperation.java @@ -1,5 +1,5 @@ /* - * Copyright 2014-2016 the original author or authors. + * Copyright 2014-2017 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -22,7 +22,7 @@ package org.springframework.data.rest.webmvc.json.patch; * @author Craig Walls * @author Oliver Gierke */ -public class RemoveOperation extends PatchOperation { +class RemoveOperation extends PatchOperation { /** * Constructs the remove operation @@ -35,10 +35,10 @@ public class RemoveOperation extends PatchOperation { /* * (non-Javadoc) - * @see org.springframework.data.rest.webmvc.json.patch.PatchOperation#perform(java.lang.Object, java.lang.Class) + * @see org.springframework.data.rest.webmvc.json.patch.PatchOperation#doPerform(java.lang.Object, java.lang.Class) */ @Override - void perform(Object target, Class type) { + void doPerform(Object target, Class type) { popValueAtPath(target, path); } } diff --git a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/ReplaceOperation.java b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/ReplaceOperation.java index 6629f4c1b..fcfbfac03 100644 --- a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/ReplaceOperation.java +++ b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/ReplaceOperation.java @@ -1,5 +1,5 @@ /* - * Copyright 2014 the original author or authors. + * Copyright 2014-2017 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -19,8 +19,9 @@ package org.springframework.data.rest.webmvc.json.patch; * Operation that replaces the value at the given path with a new value. * * @author Craig Walls + * @author Oliver Gierke */ -public class ReplaceOperation extends PatchOperation { +class ReplaceOperation extends PatchOperation { /** * Constructs the replace operation @@ -34,10 +35,10 @@ public class ReplaceOperation extends PatchOperation { /* * (non-Javadoc) - * @see org.springframework.data.rest.webmvc.json.patch.PatchOperation#perform(java.lang.Object, java.lang.Class) + * @see org.springframework.data.rest.webmvc.json.patch.PatchOperation#doPerform(java.lang.Object, java.lang.Class) */ @Override - void perform(Object target, Class type) { + void doPerform(Object target, Class type) { setValueOnTarget(target, evaluateValueFromTarget(target, type)); } } diff --git a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/TestOperation.java b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/TestOperation.java index 9c2681225..1d2ad3acc 100644 --- a/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/TestOperation.java +++ b/spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/json/patch/TestOperation.java @@ -1,5 +1,5 @@ /* - * Copyright 2014-2016 the original author or authors. + * Copyright 2014-2017 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -30,6 +30,7 @@ import org.springframework.util.ObjectUtils; *

* * @author Craig Walls + * @author Oliver Gierke */ class TestOperation extends PatchOperation { @@ -45,10 +46,10 @@ class TestOperation extends PatchOperation { /* * (non-Javadoc) - * @see org.springframework.data.rest.webmvc.json.patch.PatchOperation#perform(java.lang.Object, java.lang.Class) + * @see org.springframework.data.rest.webmvc.json.patch.PatchOperation#doPerform(java.lang.Object, java.lang.Class) */ @Override - void perform(Object target, Class type) { + void doPerform(Object target, Class type) { Object expected = normalizeIfNumber(evaluateValueFromTarget(target, type)); Object actual = normalizeIfNumber(getValueFromTarget(target)); @@ -58,7 +59,7 @@ class TestOperation extends PatchOperation { } } - private Object normalizeIfNumber(Object expected) { + private static Object normalizeIfNumber(Object expected) { if (expected instanceof Double || expected instanceof Float) { expected = BigDecimal.valueOf(((Number) expected).doubleValue()); diff --git a/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/JsonPatchTests.java b/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/JsonPatchTests.java index 55d9e2af7..f4057828a 100755 --- a/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/JsonPatchTests.java +++ b/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/JsonPatchTests.java @@ -15,7 +15,9 @@ */ package org.springframework.data.rest.webmvc.json.patch; +import static org.assertj.core.api.Assertions.*; import static org.junit.Assert.*; +import static org.junit.Assert.fail; import java.io.IOException; import java.math.BigInteger; @@ -162,9 +164,12 @@ public class JsonPatchTests { patch.apply(todo, Todo.class); } - @Test(expected = PatchException.class) // DATAREST-1127 + @Test // DATAREST-1127 public void rejectsInvalidPaths() throws Exception { - readJsonPatch("patch-invalid-path.json").apply(new Todo(), Todo.class); + + assertThatExceptionOfType(PatchException.class).isThrownBy(() -> { + readJsonPatch("patch-invalid-path.json").apply(new Todo(), Todo.class); + }); } private Patch readJsonPatch(String jsonPatchFile) throws IOException, JsonParseException, JsonMappingException { diff --git a/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/PatchOperationUnitTests.java b/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/PatchOperationUnitTests.java new file mode 100644 index 000000000..87a47a49a --- /dev/null +++ b/spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/json/patch/PatchOperationUnitTests.java @@ -0,0 +1,49 @@ +/* + * Copyright 2017 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.springframework.data.rest.webmvc.json.patch; + +import static org.assertj.core.api.Assertions.*; + +import org.junit.Test; + +/** + * General unit tests for {@link PatchOperation} implementations. + * + * @author Oliver Gierke + */ +public class PatchOperationUnitTests { + + @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) // + .isThrownBy(() -> operation.perform(todo, Todo.class)); + } +}