From ab8f2adaf595afa759c781b456d80eb010fd2b0f Mon Sep 17 00:00:00 2001 From: Tomer Gabel Date: Fri, 13 Dec 2024 21:56:30 +0200 Subject: [PATCH] Throw on root node replacement (see #188) (#190) * Reimplement full test suite with applyInPlace * Support exceptions for InPlaceApplyProcessor + tests --- README.md | 5 +++ .../zjsonpatch/CopyingApplyProcessor.java | 5 +++ .../zjsonpatch/InPlaceApplyProcessor.java | 27 +++++++++----- .../com/flipkart/zjsonpatch/AbstractTest.java | 36 ++++++++++++++----- .../flipkart/zjsonpatch/PatchTestCase.java | 5 +++ .../resources/testdata/js-libs-samples.json | 9 +++-- src/test/resources/testdata/replace.json | 3 +- 7 files changed, 68 insertions(+), 22 deletions(-) diff --git a/README.md b/README.md index afc9c18..b09cd99 100644 --- a/README.md +++ b/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(). diff --git a/src/main/java/com/flipkart/zjsonpatch/CopyingApplyProcessor.java b/src/main/java/com/flipkart/zjsonpatch/CopyingApplyProcessor.java index 53bf185..8dc3de6 100644 --- a/src/main/java/com/flipkart/zjsonpatch/CopyingApplyProcessor.java +++ b/src/main/java/com/flipkart/zjsonpatch/CopyingApplyProcessor.java @@ -12,4 +12,9 @@ class CopyingApplyProcessor extends InPlaceApplyProcessor { CopyingApplyProcessor(JsonNode target, EnumSet flags) { super(target.deepCopy(), flags); } + + @Override + protected boolean allowRootReplacement() { + return true; + } } diff --git a/src/main/java/com/flipkart/zjsonpatch/InPlaceApplyProcessor.java b/src/main/java/com/flipkart/zjsonpatch/InPlaceApplyProcessor.java index a63b03b..f77951c 100644 --- a/src/main/java/com/flipkart/zjsonpatch/InPlaceApplyProcessor.java +++ b/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) { diff --git a/src/test/java/com/flipkart/zjsonpatch/AbstractTest.java b/src/test/java/com/flipkart/zjsonpatch/AbstractTest.java index 85a94a3..019b5a4 100644 --- a/src/test/java/com/flipkart/zjsonpatch/AbstractTest.java +++ b/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,15 +45,24 @@ 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"); @@ -62,7 +70,13 @@ private void testOperation() throws Exception { 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()) { diff --git a/src/test/java/com/flipkart/zjsonpatch/PatchTestCase.java b/src/test/java/com/flipkart/zjsonpatch/PatchTestCase.java index 03c8328..2b98a1b 100644 --- a/src/test/java/com/flipkart/zjsonpatch/PatchTestCase.java +++ b/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()); + } } diff --git a/src/test/resources/testdata/js-libs-samples.json b/src/test/resources/testdata/js-libs-samples.json index 51ce825..18b25fb 100644 --- a/src/test/resources/testdata/js-libs-samples.json +++ b/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"}], diff --git a/src/test/resources/testdata/replace.json b/src/test/resources/testdata/replace.json index e5a9abe..b0ecfc9 100644 --- a/src/test/resources/testdata/replace.json +++ b/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" }],