Throw on root node replacement (see #188) (#190)

* Reimplement full test suite with applyInPlace

* Support exceptions for InPlaceApplyProcessor + tests

Author: holograph

README.md

@@ -86,6 +86,11 @@ JsonPatch.applyInPlace(JsonNode patch, JsonNode source);
 Given a `patch`, it will apply it to the `source` JSON mutating the instance, opposed to `JsonPatch.apply` which returns 
 a new instance with the patch applied, leaving the `source` unchanged.
 
+This is an extension to the RFC, and has some additional limitations. Specifically, the source document cannot be fully change in place (the Jackson APIs do not support that level of mutability). This means the following operations are not supported:
+* `remove` with an empty or root path;
+* `replace` with an empty or root path;
+* `move`, `add` or `copy` targeting an empty or root path. 
+
 ### Tests:
 1. 100+ selective hardcoded different input JSONs , with their driver test classes present under /test directory.
 2. Apart from selective input, a deterministic random JSON generator is present under ( TestDataGenerator.java ),  and its driver test class method is JsonDiffTest.testGeneratedJsonDiff().

src/main/java/com/flipkart/zjsonpatch/CopyingApplyProcessor.java

@@ -12,4 +12,9 @@ class CopyingApplyProcessor extends InPlaceApplyProcessor {
     CopyingApplyProcessor(JsonNode target, EnumSet<CompatibilityFlags> flags) {
         super(target.deepCopy(), flags);
     }
+
+    @Override
+    protected boolean allowRootReplacement() {
+        return true;
+    }
 }

src/main/java/com/flipkart/zjsonpatch/InPlaceApplyProcessor.java

@@ -40,6 +40,10 @@ public JsonNode result() {
         return target;
     }
 
+    protected boolean allowRootReplacement() {
+        return false;
+    }
+
     @Override
     public void move(JsonPointer fromPath, JsonPointer toPath) throws JsonPointerEvaluationException {
         JsonNode valueNode = fromPath.evaluate(target);
@@ -81,6 +85,8 @@ public void add(JsonPointer path, JsonNode value) throws JsonPointerEvaluationEx
     @Override
     public void replace(JsonPointer path, JsonNode value) throws JsonPointerEvaluationException {
         if (path.isRoot()) {
+            if (!allowRootReplacement())
+                throw new JsonPatchApplicationException("Cannot replace root document", Operation.REPLACE, path);
             target = value;
             return;
         }
@@ -132,17 +138,20 @@ else if (parentNode.isArray()) {
 
 
     private void set(JsonPointer path, JsonNode value, Operation forOp) throws JsonPointerEvaluationException {
-        if (path.isRoot())
+        if (path.isRoot()) {
+            if (!allowRootReplacement())
+                throw new JsonPatchApplicationException("Cannot replace root document", forOp, path);
             target = value;
-        else {
-            JsonNode parentNode = path.getParent().evaluate(target);
-            if (!parentNode.isContainerNode())
-                throw new JsonPatchApplicationException("Cannot reference past scalar value", forOp, path.getParent());
-            else if (parentNode.isArray())
-                addToArray(path, value, parentNode);
-            else
-                addToObject(path, parentNode, value);
+            return;
         }
+
+        JsonNode parentNode = path.getParent().evaluate(target);
+        if (!parentNode.isContainerNode())
+            throw new JsonPatchApplicationException("Cannot reference past scalar value", forOp, path.getParent());
+        else if (parentNode.isArray())
+            addToArray(path, value, parentNode);
+        else
+            addToObject(path, parentNode, value);
     }
 
     private void addToObject(JsonPointer path, JsonNode node, JsonNode value) {

src/test/java/com/flipkart/zjsonpatch/AbstractTest.java

@@ -28,7 +28,6 @@
 import java.io.PrintWriter;
 import java.io.StringWriter;
 
-import static org.hamcrest.core.IsEqual.equalTo;
 import static org.hamcrest.core.IsInstanceOf.instanceOf;
 import static org.hamcrest.core.StringContains.containsString;
 import static org.junit.Assert.assertEquals;
@@ -46,23 +45,38 @@ protected boolean matchOnErrors() {
     }
 
     @Test
-    public void test() throws Exception {
+    public void testApply() throws Exception {
         if (p.isOperation()) {
-            testOperation();
+            testOperation(false);
         } else {
-            testError();
+            testError(false);
         }
     }
 
-    private void testOperation() throws Exception {
+    @Test
+    public void testApplyInPlace() throws Exception {
+        if (p.isOperation() && p.isApplyInPlaceSupported()) {
+            testOperation(true);
+        } else {
+            testError(true);
+        }
+    }
+
+    private void testOperation(boolean inPlace) {
         JsonNode node = p.getNode();
 
         JsonNode doc = node.get("node");
         JsonNode expected = node.get("expected");
         JsonNode patch = node.get("op");
         String message = node.has("message") ? node.get("message").toString() : "";
 
-        JsonNode result = JsonPatch.apply(patch, doc);
+        JsonNode result;
+        if (inPlace) {
+            result = doc.deepCopy();
+            JsonPatch.applyInPlace(patch, result);
+        } else {
+            result = JsonPatch.apply(patch, doc);
+        }
         String failMessage = "The following test failed: \n" +
                 "message: " + message + '\n' +
                 "at: " + p.getSourceFile();
@@ -91,7 +105,7 @@ private String errorMessage(String header, Exception e) throws JsonProcessingExc
         return res.toString();
     }
 
-    private void testError() throws JsonProcessingException, ClassNotFoundException {
+    private void testError(boolean inPlace) throws JsonProcessingException, ClassNotFoundException {
         JsonNode node = p.getNode();
         JsonNode first = node.get("node");
         JsonNode patch = node.get("op");
@@ -100,8 +114,12 @@ private void testError() throws JsonProcessingException, ClassNotFoundException
                 node.has("type") ? exceptionType(node.get("type").textValue()) : JsonPatchApplicationException.class;
 
         try {
-            JsonPatch.apply(patch, first);
-
+            if (inPlace) {
+                JsonNode target = first.deepCopy();
+                JsonPatch.applyInPlace(patch, target);
+            } else {
+                JsonPatch.apply(patch, first);
+            }
             fail(errorMessage("Failure expected: " + message));
         } catch (Exception e) {
             if (matchOnErrors()) {

src/test/java/com/flipkart/zjsonpatch/PatchTestCase.java

@@ -69,4 +69,9 @@ private static boolean isEnabled(JsonNode node) {
         JsonNode disabled = node.get("disabled");
         return (disabled == null || !disabled.booleanValue());
     }
+
+    public boolean isApplyInPlaceSupported() {
+        JsonNode allowInPlace = node.get("allowInPlace");
+        return (allowInPlace == null || allowInPlace.booleanValue());
+    }
 }

src/test/resources/testdata/js-libs-samples.json

@@ -75,12 +75,14 @@
     { "message": "replacing the root of the document is possible with add",
       "node": {"foo": "bar"},
       "op": [{"op": "add", "path": "", "value": {"baz": "qux"}}],
-      "expected": {"baz":"qux"}},
+      "expected": {"baz":"qux"},
+      "allowInPlace": false },
 
     { "message": "replacing the root of the document is possible with add",
       "node": {"foo": "bar"},
       "op": [{"op": "add", "path": "", "value": ["baz", "qux"]}],
-      "expected": ["baz", "qux"]},
+      "expected": ["baz", "qux"],
+      "allowInPlace": false },
 
     { "message": "empty list, empty docs",
       "node": {},
@@ -235,7 +237,8 @@
     { "message": "replace whole document",
       "node": {"foo": "bar"},
       "op": [{"op": "replace", "path": "", "value": {"baz": "qux"}}],
-      "expected": {"baz": "qux"} },
+      "expected": {"baz": "qux"},
+      "allowInPlace": false },
 
     { "node": {"foo": null},
       "op": [{"op": "replace", "path": "/foo", "value": "truthy"}],

src/test/resources/testdata/replace.json

@@ -21,7 +21,8 @@
         {
             "op": [{ "op": "replace", "path": "", "value": false }],
             "node": { "x": { "a": "b", "y": {} } },
-            "expected": false
+            "expected": false,
+            "allowInPlace": false
         },
         {
             "op": [{ "op": "replace", "path": "/x/y", "value": "hello" }],