diff --git a/src/main/java/com/flipkart/zjsonpatch/InPlaceApplyProcessor.java b/src/main/java/com/flipkart/zjsonpatch/InPlaceApplyProcessor.java index 1c9ac9b..f40a069 100644 --- a/src/main/java/com/flipkart/zjsonpatch/InPlaceApplyProcessor.java +++ b/src/main/java/com/flipkart/zjsonpatch/InPlaceApplyProcessor.java @@ -37,16 +37,16 @@ public JsonNode result() { @Override public void move(List fromPath, List toPath) { - JsonNode parentNode = getParentNode(fromPath); + JsonNode parentNode = getParentNode(fromPath, Operation.MOVE); String field = fromPath.get(fromPath.size() - 1).replaceAll("\"", ""); - JsonNode valueNode = parentNode.isArray() ? parentNode.get(Integer.parseInt(field)) : parentNode.get(field); + JsonNode valueNode = parentNode.isArray() ? parentNode.get(Integer.parseInt(field)) : parentNode.get(field); remove(fromPath); add(toPath, valueNode); } @Override public void copy(List fromPath, List toPath) { - JsonNode parentNode = getParentNode(fromPath); + JsonNode parentNode = getParentNode(fromPath, Operation.COPY); String field = fromPath.get(fromPath.size() - 1).replaceAll("\"", ""); JsonNode valueNode = parentNode.isArray() ? parentNode.get(Integer.parseInt(field)) : parentNode.get(field); add(toPath, valueNode); @@ -55,67 +55,61 @@ public void copy(List fromPath, List toPath) { @Override public void test(List path, JsonNode value) { if (path.isEmpty()) { - throw new JsonPatchApplicationException("[TEST Operation] path is empty , path : "); + error(Operation.TEST, "path is empty , path : "); } else { - JsonNode parentNode = getParentNode(path); - if (parentNode == null) { - throw new JsonPatchApplicationException("[TEST Operation] noSuchPath in source, path provided : " + path); - } else { - String fieldToReplace = path.get(path.size() - 1).replaceAll("\"", ""); - if (fieldToReplace.equals("") && path.size() == 1) - if(target.equals(value)){ - target = value; - }else { - throw new JsonPatchApplicationException("[TEST Operation] value mismatch"); - } - else if (!parentNode.isContainerNode()) - throw new JsonPatchApplicationException("[TEST Operation] parent is not a container in source, path provided : " + path + " | node : " + parentNode); - else if (parentNode.isArray()) { - final ArrayNode target = (ArrayNode) parentNode; - String idxStr = path.get(path.size() - 1); - - if ("-".equals(idxStr)) { - // see http://tools.ietf.org/html/rfc6902#section-4.1 - if(!target.get(target.size()-1).equals(value)){ - throw new JsonPatchApplicationException("[TEST Operation] value mismatch"); - } - } else { - int idx = arrayIndex(idxStr.replaceAll("\"", ""), target.size()); - if(!target.get(idx).equals(value)){ - throw new JsonPatchApplicationException("[TEST Operation] value mismatch"); - } - } + JsonNode parentNode = getParentNode(path, Operation.TEST); + String fieldToReplace = path.get(path.size() - 1).replaceAll("\"", ""); + if (fieldToReplace.equals("") && path.size() == 1) + if(target.equals(value)){ + target = value; + }else { + error(Operation.TEST, "value mismatch"); } - else { - final ObjectNode target = (ObjectNode) parentNode; - String key = path.get(path.size() - 1).replaceAll("\"", ""); - if(!target.get(key).equals(value)){ - throw new JsonPatchApplicationException("[TEST Operation] value mismatch"); + else if (!parentNode.isContainerNode()) + error(Operation.TEST, "parent is not a container in source, path provided : " + path + " | node : " + parentNode); + else if (parentNode.isArray()) { + final ArrayNode target = (ArrayNode) parentNode; + String idxStr = path.get(path.size() - 1); + + if ("-".equals(idxStr)) { + // see http://tools.ietf.org/html/rfc6902#section-4.1 + if(!target.get(target.size()-1).equals(value)){ + error(Operation.TEST, "value mismatch"); + } + } else { + int idx = arrayIndex(idxStr.replaceAll("\"", ""), target.size()); + if(!target.get(idx).equals(value)){ + error(Operation.TEST, "value mismatch"); } } } + else { + final ObjectNode target = (ObjectNode) parentNode; + String key = path.get(path.size() - 1).replaceAll("\"", ""); + JsonNode actual = target.get(key); + if (actual == null) + error(Operation.TEST, "noSuchPath in source, path provided : " + path); + else if (!actual.equals(value)) + error(Operation.TEST, "value mismatch"); + } } } @Override public void add(List path, JsonNode value) { if (path.isEmpty()) { - throw new JsonPatchApplicationException("[ADD Operation] path is empty , path : "); + error(Operation.ADD, "path is empty , path : "); } else { - JsonNode parentNode = getParentNode(path); - if (parentNode == null) { - throw new JsonPatchApplicationException("[ADD Operation] noSuchPath in source, path provided : " + path); - } else { - String fieldToReplace = path.get(path.size() - 1).replaceAll("\"", ""); - if (fieldToReplace.equals("") && path.size() == 1) - target = value; - else if (!parentNode.isContainerNode()) - throw new JsonPatchApplicationException("[ADD Operation] parent is not a container in source, path provided : " + path + " | node : " + parentNode); - else if (parentNode.isArray()) - addToArray(path, value, parentNode); - else - addToObject(path, parentNode, value); - } + JsonNode parentNode = getParentNode(path, Operation.ADD); + String fieldToReplace = path.get(path.size() - 1).replaceAll("\"", ""); + if (fieldToReplace.equals("") && path.size() == 1) + target = value; + else if (!parentNode.isContainerNode()) + error(Operation.ADD, "parent is not a container in source, path provided : " + path + " | node : " + parentNode); + else if (parentNode.isArray()) + addToArray(path, value, parentNode); + else + addToObject(path, parentNode, value); } } @@ -141,48 +135,46 @@ private void addToArray(List path, JsonNode value, JsonNode parentNode) @Override public void replace(List path, JsonNode value) { if (path.isEmpty()) { - throw new JsonPatchApplicationException("[Replace Operation] path is empty"); + error(Operation.REPLACE, "path is empty"); } else { - JsonNode parentNode = getParentNode(path); - if (parentNode == null) { - throw new JsonPatchApplicationException("[Replace Operation] noSuchPath in source, path provided : " + path); - } else { - String fieldToReplace = path.get(path.size() - 1).replaceAll("\"", ""); - if (Strings.isNullOrEmpty(fieldToReplace) && path.size() == 1) - target = value; - else if (parentNode.isObject()) - ((ObjectNode) parentNode).put(fieldToReplace, value); - else if (parentNode.isArray()) - ((ArrayNode) parentNode).set(arrayIndex(fieldToReplace, parentNode.size() - 1), value); - else - throw new JsonPatchApplicationException("[Replace Operation] noSuchPath in source, path provided : " + path); - } + JsonNode parentNode = getParentNode(path, Operation.REPLACE); + String fieldToReplace = path.get(path.size() - 1).replaceAll("\"", ""); + if (Strings.isNullOrEmpty(fieldToReplace) && path.size() == 1) + target = value; + else if (parentNode.isObject()) + ((ObjectNode) parentNode).put(fieldToReplace, value); + else if (parentNode.isArray()) + ((ArrayNode) parentNode).set(arrayIndex(fieldToReplace, parentNode.size() - 1), value); + else + error(Operation.REPLACE, "noSuchPath in source, path provided : " + path); } } @Override public void remove(List path) { if (path.isEmpty()) { - throw new JsonPatchApplicationException("[Remove Operation] path is empty"); + error(Operation.REMOVE, "path is empty"); } else { - JsonNode parentNode = getParentNode(path); - if (parentNode == null) { - throw new JsonPatchApplicationException("[Remove Operation] noSuchPath in source, path provided : " + path); - } else { - String fieldToRemove = path.get(path.size() - 1).replaceAll("\"", ""); - if (parentNode.isObject()) - ((ObjectNode) parentNode).remove(fieldToRemove); - else if (parentNode.isArray()) - ((ArrayNode) parentNode).remove(arrayIndex(fieldToRemove, parentNode.size() - 1)); - else - throw new JsonPatchApplicationException("[Remove Operation] noSuchPath in source, path provided : " + path); - } + JsonNode parentNode = getParentNode(path, Operation.REMOVE); + String fieldToRemove = path.get(path.size() - 1).replaceAll("\"", ""); + if (parentNode.isObject()) + ((ObjectNode) parentNode).remove(fieldToRemove); + else if (parentNode.isArray()) + ((ArrayNode) parentNode).remove(arrayIndex(fieldToRemove, parentNode.size() - 1)); + else + error(Operation.REMOVE, "noSuchPath in source, path provided : " + path); } } - private JsonNode getParentNode(List fromPath) { + private void error(Operation forOp, String message) { + throw new JsonPatchApplicationException("[" + forOp + " Operation] " + message); + } + + private JsonNode getParentNode(List fromPath, Operation forOp) { List pathToParent = fromPath.subList(0, fromPath.size() - 1); // would never by out of bound, lets see - return getNode(target, pathToParent, 1); + JsonNode node = getNode(target, pathToParent, 1); + if (node == null) error(forOp, "noSuchPath in source, path provided: " + fromPath); + return node; } private JsonNode getNode(JsonNode ret, List path, int pos) { @@ -208,7 +200,12 @@ private JsonNode getNode(JsonNode ret, List path, int pos) { } private int arrayIndex(String s, int max) { - int index = Integer.parseInt(s); + int index; + try { + index = Integer.parseInt(s); + } catch (NumberFormatException nfe) { + throw new JsonPatchApplicationException("Object operation on array target"); + } if (index < 0) { throw new JsonPatchApplicationException("index Out of bound, index is negative"); } else if (index > max) { diff --git a/src/test/java/com/flipkart/zjsonpatch/AbstractTest.java b/src/test/java/com/flipkart/zjsonpatch/AbstractTest.java index c4266e1..060df98 100644 --- a/src/test/java/com/flipkart/zjsonpatch/AbstractTest.java +++ b/src/test/java/com/flipkart/zjsonpatch/AbstractTest.java @@ -16,13 +16,21 @@ package com.flipkart.zjsonpatch; +import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; +import org.apache.commons.io.output.StringBuilderWriter; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.Parameterized; import org.junit.runners.Parameterized.Parameter; +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.assertThat; import static org.junit.Assert.fail; @@ -32,16 +40,20 @@ public abstract class AbstractTest { @Parameter public PatchTestCase p; + protected boolean matchOnErrors() { + return true; + } + @Test public void test() throws Exception { if (p.isOperation()) { - testOpertaion(); + testOperation(); } else { testError(); } } - private void testOpertaion() throws Exception { + private void testOperation() throws Exception { JsonNode node = p.getNode(); JsonNode first = node.get("node"); @@ -54,17 +66,56 @@ private void testOpertaion() throws Exception { assertThat(message, secondPrime, equalTo(second)); } - private void testError() { - JsonNode node = p.getNode(); + private Class exceptionType(String type) throws ClassNotFoundException { + return Class.forName(type.contains(".") ? type : "com.flipkart.zjsonpatch." + type); + } + private String errorMessage(String header) throws JsonProcessingException { + return errorMessage(header, null); + } + private String errorMessage(String header, Exception e) throws JsonProcessingException { + StringBuilder res = + new StringBuilder() + .append(header) + .append("\nFull test case (in file ") + .append(p.getSourceFile()) + .append("):\n") + .append(new ObjectMapper().writerWithDefaultPrettyPrinter().writeValueAsString(p.getNode())); + if (e != null) { + res.append("\nFull error: "); + e.printStackTrace(new PrintWriter(new StringBuilderWriter(res))); + } + return res.toString(); + } + + private void testError() throws JsonProcessingException, ClassNotFoundException { + JsonNode node = p.getNode(); JsonNode first = node.get("node"); JsonNode patch = node.get("op"); + JsonNode message = node.get("message"); + Class type = + node.has("type") ? exceptionType(node.get("type").textValue()) : JsonPatchApplicationException.class; try { JsonPatch.apply(patch, first); - fail("Failure expected: " + node.get("message")); - } catch (Exception ex) { + fail(errorMessage("Failure expected: " + message)); + } catch (Exception e) { + if (matchOnErrors()) { + StringWriter fullError = new StringWriter(); + e.printStackTrace(new PrintWriter(fullError)); + + assertThat( + errorMessage("Operation failed but with wrong exception type", e), + e, + instanceOf(type)); + if (message != null) { + assertThat( + errorMessage("Operation failed but with wrong message", e), + e.getMessage(), + containsString(message.textValue())); // equalTo would be better, but fail existing tests + } + } } } } diff --git a/src/test/java/com/flipkart/zjsonpatch/JsLibSamplesTest.java b/src/test/java/com/flipkart/zjsonpatch/JsLibSamplesTest.java index 78138af..c78e33c 100644 --- a/src/test/java/com/flipkart/zjsonpatch/JsLibSamplesTest.java +++ b/src/test/java/com/flipkart/zjsonpatch/JsLibSamplesTest.java @@ -16,9 +16,10 @@ package com.flipkart.zjsonpatch; +import org.junit.runners.Parameterized; + import java.io.IOException; import java.util.Collection; -import org.junit.runners.Parameterized; /** * @author ctranxuan (streamdata.io). @@ -33,4 +34,9 @@ public class JsLibSamplesTest extends AbstractTest { public static Collection data() throws IOException { return PatchTestCase.load("js-libs-samples"); } + + @Override + protected boolean matchOnErrors() { + return false; + } } diff --git a/src/test/java/com/flipkart/zjsonpatch/PatchTestCase.java b/src/test/java/com/flipkart/zjsonpatch/PatchTestCase.java index 01be8d3..b121ad8 100644 --- a/src/test/java/com/flipkart/zjsonpatch/PatchTestCase.java +++ b/src/test/java/com/flipkart/zjsonpatch/PatchTestCase.java @@ -18,21 +18,24 @@ import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.ObjectMapper; +import org.apache.commons.io.IOUtils; + import java.io.IOException; import java.io.InputStream; import java.util.ArrayList; import java.util.Collection; import java.util.List; -import org.apache.commons.io.IOUtils; public class PatchTestCase { private final boolean operation; private final JsonNode node; + private final String sourceFile; - private PatchTestCase(boolean isOperation, JsonNode node) { + private PatchTestCase(boolean isOperation, JsonNode node, String sourceFile) { this.operation = isOperation; this.node = node; + this.sourceFile = sourceFile; } public boolean isOperation() { @@ -43,6 +46,10 @@ public JsonNode getNode() { return node; } + public String getSourceFile() { + return sourceFile; + } + private static final ObjectMapper MAPPER = new ObjectMapper(); public static Collection load(String fileName) throws IOException { @@ -54,12 +61,12 @@ public static Collection load(String fileName) throws IOException List result = new ArrayList(); for (JsonNode node : tree.get("errors")) { if (isEnabled(node)) { - result.add(new PatchTestCase(false, node)); + result.add(new PatchTestCase(false, node, path)); } } for (JsonNode node : tree.get("ops")) { if (isEnabled(node)) { - result.add(new PatchTestCase(true, node)); + result.add(new PatchTestCase(true, node, path)); } } return result; diff --git a/src/test/java/com/flipkart/zjsonpatch/Rfc6902SamplesTest.java b/src/test/java/com/flipkart/zjsonpatch/Rfc6902SamplesTest.java index 30d2ba1..4a57651 100644 --- a/src/test/java/com/flipkart/zjsonpatch/Rfc6902SamplesTest.java +++ b/src/test/java/com/flipkart/zjsonpatch/Rfc6902SamplesTest.java @@ -16,9 +16,10 @@ package com.flipkart.zjsonpatch; +import org.junit.runners.Parameterized; + import java.io.IOException; import java.util.Collection; -import org.junit.runners.Parameterized; /** * @author ctranxuan (streamdata.io). @@ -29,4 +30,11 @@ public class Rfc6902SamplesTest extends AbstractTest { public static Collection data() throws IOException { return PatchTestCase.load("rfc6902-samples"); } + + @Override + protected boolean matchOnErrors() { + // Error matching disabled to avoid a lot of rote work on the samples. + // TODO revisit samples and possibly change "message" fields to "reference" or something more descriptive + return false; + } } diff --git a/src/test/resources/testdata/add.json b/src/test/resources/testdata/add.json index 43307da..748d7e7 100644 --- a/src/test/resources/testdata/add.json +++ b/src/test/resources/testdata/add.json @@ -3,7 +3,8 @@ { "op": [{ "op": "add", "path": "/a" }], "node": {}, - "message": "Missing value field on add operation" + "type": "JsonPatchApplicationException", + "message": "missing 'value' field" } ], "ops": [ diff --git a/src/test/resources/testdata/copy.json b/src/test/resources/testdata/copy.json index 1ccf1f6..ac29607 100644 --- a/src/test/resources/testdata/copy.json +++ b/src/test/resources/testdata/copy.json @@ -3,7 +3,7 @@ { "op": [{ "op": "copy", "from": "/a", "path": "/b/c" }], "node": { "a": 1 }, - "message": "jsonPatch.noSuchParent" + "message": "noSuchPath in source" } ], "ops": [ diff --git a/src/test/resources/testdata/move.json b/src/test/resources/testdata/move.json index 70eae16..afdc8fc 100644 --- a/src/test/resources/testdata/move.json +++ b/src/test/resources/testdata/move.json @@ -3,17 +3,24 @@ { "op": [{ "op": "move", "from": "/a", "path": "/a/b" }], "node": {}, - "message": "jsonPatch.noSuchPath" + "message": "noSuchPath" }, { "op": [{ "op": "move", "from": "/a", "path": "/b/c" }], "node": { "a": "b" }, - "message": "jsonPatch.noSuchParent" + "message": "noSuchPath" }, { "op": [{ "op": "move", "path": "/b/c" }], "node": { "a": "b" }, - "message": "Missing from field" + "type": "InvalidJsonPatchException", + "message": "missing 'from' field" + }, + { + "issue": 39, + "op": [{ "op": "move", "from": "/1/key", "path": "/0/key" }], + "node": [{ "key": "0" }], + "message": "noSuchPath" } ], "ops": [ diff --git a/src/test/resources/testdata/replace.json b/src/test/resources/testdata/replace.json index ac0fd42..60764f1 100644 --- a/src/test/resources/testdata/replace.json +++ b/src/test/resources/testdata/replace.json @@ -3,7 +3,8 @@ { "op": [{ "op": "replace", "path": "/a" }], "node": { "a": 0 }, - "message": "Missing value field" + "type": "InvalidJsonPatchException", + "message": "missing 'value' field" }, { "op": [{ "op": "replace", "path": "/x/y", "value": false }], diff --git a/src/test/resources/testdata/test.json b/src/test/resources/testdata/test.json index aa8472b..8b3d491 100644 --- a/src/test/resources/testdata/test.json +++ b/src/test/resources/testdata/test.json @@ -1,19 +1,24 @@ { "errors": [ + { + "op": [{ "op": "test", "path": "/x", "value": {} }], + "node": { "key": "value" }, + "message": "noSuchPath" + }, { "op": [{ "op": "test", "path": "/x", "value": {} }], "node": [ 1, 2 ], - "message": "jsonPatch.noSuchPath" + "message": "Object operation on array target" }, { "op": [{ "op": "test", "path": "", "value": true }], "node": [ 1, 2 ], - "message": "jsonPatch.valueTestFailure" + "message": "value mismatch" }, { "op": [{ "op": "test", "path": "/x", "value": -30.000 }], "node": { "x": -29.020 }, - "message": "jsonPatch.valueTestFailure" + "message": "value mismatch" } ], "ops": [