From d2c4e4e860a21aac782af37362d3e56812e88642 Mon Sep 17 00:00:00 2001 From: Tomer Gabel Date: Tue, 25 Jul 2017 15:05:50 +0300 Subject: [PATCH 01/14] Typo + added actual error message test condition --- .../com/flipkart/zjsonpatch/AbstractTest.java | 20 +++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/src/test/java/com/flipkart/zjsonpatch/AbstractTest.java b/src/test/java/com/flipkart/zjsonpatch/AbstractTest.java index c4266e1..2ff4677 100644 --- a/src/test/java/com/flipkart/zjsonpatch/AbstractTest.java +++ b/src/test/java/com/flipkart/zjsonpatch/AbstractTest.java @@ -16,13 +16,16 @@ package com.flipkart.zjsonpatch; +import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.Parameterized; import org.junit.runners.Parameterized.Parameter; import static org.hamcrest.core.IsEqual.equalTo; +import static org.hamcrest.core.StringContains.containsString; import static org.junit.Assert.assertThat; import static org.junit.Assert.fail; @@ -35,13 +38,13 @@ public abstract class AbstractTest { @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 +57,26 @@ private void testOpertaion() throws Exception { assertThat(message, secondPrime, equalTo(second)); } - private void testError() { + private void testError() throws JsonProcessingException { JsonNode node = p.getNode(); JsonNode first = node.get("node"); JsonNode patch = node.get("op"); + JsonNode message = node.get("message"); try { JsonPatch.apply(patch, first); fail("Failure expected: " + node.get("message")); - } catch (Exception ex) { + } catch (JsonPatchApplicationException e) { + if (message != null) { + + assertThat( + "Operation failed but with wrong message for test case:\n" + + new ObjectMapper().writerWithDefaultPrettyPrinter().writeValueAsString(node), + e.getMessage(), + containsString(message.textValue())); // equalTo would be better, but fail existing tests + } } } } From 8285945b82fa7094acd0d9788555ae741b0f91d7 Mon Sep 17 00:00:00 2001 From: Tomer Gabel Date: Tue, 25 Jul 2017 16:08:42 +0300 Subject: [PATCH 02/14] Added support for matching on exception types + better errors --- .../com/flipkart/zjsonpatch/AbstractTest.java | 26 ++++++++++++++----- .../flipkart/zjsonpatch/PatchTestCase.java | 15 ++++++++--- 2 files changed, 30 insertions(+), 11 deletions(-) diff --git a/src/test/java/com/flipkart/zjsonpatch/AbstractTest.java b/src/test/java/com/flipkart/zjsonpatch/AbstractTest.java index 2ff4677..537e293 100644 --- a/src/test/java/com/flipkart/zjsonpatch/AbstractTest.java +++ b/src/test/java/com/flipkart/zjsonpatch/AbstractTest.java @@ -26,6 +26,7 @@ import static org.hamcrest.core.IsEqual.equalTo; import static org.hamcrest.core.StringContains.containsString; +import static org.hamcrest.core.IsInstanceOf.instanceOf; import static org.junit.Assert.assertThat; import static org.junit.Assert.fail; @@ -57,23 +58,34 @@ private void testOperation() throws Exception { assertThat(message, secondPrime, equalTo(second)); } - private void testError() throws JsonProcessingException { - 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 { + String testCase = new ObjectMapper().writerWithDefaultPrettyPrinter().writeValueAsString(p.getNode()); + return header + "\nFull test case (in file " + p.getSourceFile() + "):\n" + testCase; + } + 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 (JsonPatchApplicationException e) { - if (message != null) { + fail(errorMessage("Failure expected: " + message)); + } catch (Exception e) { + assertThat(errorMessage("Operation failed but with wrong exception type"), e, instanceOf(type)); + + if (message != null) { assertThat( - "Operation failed but with wrong message for test case:\n" + - new ObjectMapper().writerWithDefaultPrettyPrinter().writeValueAsString(node), + errorMessage("Operation failed but with wrong message"), e.getMessage(), containsString(message.textValue())); // equalTo would be better, but fail existing tests } 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; From 9a3461c306050df7e69245947c2db30330940cb5 Mon Sep 17 00:00:00 2001 From: Tomer Gabel Date: Tue, 25 Jul 2017 16:09:06 +0300 Subject: [PATCH 03/14] Fixed add tests --- src/test/resources/testdata/add.json | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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": [ From 30709386abba4d79791cf9798b10a4be429e173d Mon Sep 17 00:00:00 2001 From: Tomer Gabel Date: Tue, 25 Jul 2017 16:12:09 +0300 Subject: [PATCH 04/14] Added support for disabling error matching + fixed JS compat tests --- .../com/flipkart/zjsonpatch/AbstractTest.java | 20 +++++++++++-------- .../flipkart/zjsonpatch/JsLibSamplesTest.java | 8 +++++++- 2 files changed, 19 insertions(+), 9 deletions(-) diff --git a/src/test/java/com/flipkart/zjsonpatch/AbstractTest.java b/src/test/java/com/flipkart/zjsonpatch/AbstractTest.java index 537e293..150c608 100644 --- a/src/test/java/com/flipkart/zjsonpatch/AbstractTest.java +++ b/src/test/java/com/flipkart/zjsonpatch/AbstractTest.java @@ -36,6 +36,10 @@ public abstract class AbstractTest { @Parameter public PatchTestCase p; + protected boolean matchOnErrors() { + return true; + } + @Test public void test() throws Exception { if (p.isOperation()) { @@ -80,14 +84,14 @@ private void testError() throws JsonProcessingException, ClassNotFoundException fail(errorMessage("Failure expected: " + message)); } catch (Exception e) { - - assertThat(errorMessage("Operation failed but with wrong exception type"), e, instanceOf(type)); - - if (message != null) { - assertThat( - errorMessage("Operation failed but with wrong message"), - e.getMessage(), - containsString(message.textValue())); // equalTo would be better, but fail existing tests + if (matchOnErrors()) { + assertThat(errorMessage("Operation failed but with wrong exception type"), e, instanceOf(type)); + if (message != null) { + assertThat( + errorMessage("Operation failed but with wrong message"), + 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; + } } From aedf3ef290019a1c8186f4d36effd372e5c2da52 Mon Sep 17 00:00:00 2001 From: Tomer Gabel Date: Tue, 25 Jul 2017 16:12:59 +0300 Subject: [PATCH 05/14] Fixed copy test --- src/test/resources/testdata/copy.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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": [ From 96296998e054f1774fdfef2228a8872527d7b017 Mon Sep 17 00:00:00 2001 From: Tomer Gabel Date: Tue, 25 Jul 2017 16:14:26 +0300 Subject: [PATCH 06/14] Fixed replace test --- src/test/resources/testdata/replace.json | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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 }], From 514d72e533fce30bd6b6e38bea48fbe21c85d6fd Mon Sep 17 00:00:00 2001 From: Tomer Gabel Date: Tue, 25 Jul 2017 16:15:58 +0300 Subject: [PATCH 07/14] Fixed RFC sample tests --- .../com/flipkart/zjsonpatch/Rfc6902SamplesTest.java | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) 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; + } } From c1a0eb27c3f38781e964427091fdd45168aac2e9 Mon Sep 17 00:00:00 2001 From: Tomer Gabel Date: Tue, 25 Jul 2017 16:21:12 +0300 Subject: [PATCH 08/14] Cleaned up test operation tests + found new instance of NPE on #39 --- src/test/resources/testdata/test.json | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) 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": [ From 3cb8e545bbac34791f0f9ac76b3b435295c3b78c Mon Sep 17 00:00:00 2001 From: Tomer Gabel Date: Tue, 25 Jul 2017 16:25:10 +0300 Subject: [PATCH 09/14] Fixed move tests (do not pass yet, waiting for actual implementation) + added faulty use case from #39 --- src/test/resources/testdata/move.json | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/src/test/resources/testdata/move.json b/src/test/resources/testdata/move.json index 70eae16..791c755 100644 --- a/src/test/resources/testdata/move.json +++ b/src/test/resources/testdata/move.json @@ -3,17 +3,18 @@ { "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" } ], "ops": [ @@ -41,6 +42,13 @@ "op": [{ "op": "move", "from": "/a", "path": "/b/2" }], "node": { "a": "helo", "b": [ 1, 2, 3, 4 ] }, "expected": { "b": [ 1, 2, "helo", 3, 4 ] } + }, + { + "issue": 39, + "op": [{"op":"replace","path":"/version","value":43},{"op":"move","path":"/collectionValue/0","from":"/collectionValue/1"},{"op":"add","path":"/collectionValue/1","value":{"@type":"SimpleReference","id":"9c144d02-a859-4a9c-be9f-0de6fc77d5ac","notes":"voluptatum moderatius dolorum finibus solet vituperata adhuc","description":"falli intellegat antiopam mus antiopam","version":0,"referenceValue":{"@type":"SimpleD","id":"c75d0410-9729-4688-94fc-d5416a2f716e","notes":"condimentum solum simul postea noluisse altera quam eruditi","description":"sem egestas senserit curabitur autem","version":0,"booleanValue":true,"type":"SIMPLE_D"},"type":"SIMPLE_REFERENCE"}},{"op":"add","path":"/collectionValue/4","value":{"@type":"SimpleB","id":"e1011e30-9bd5-470e-a40a-4c3d7b621e16","notes":"liber vix elitr interpretaris aliquip dico quidam","description":"aenean adversarium vivamus id porta curabitur placerat invidunt","version":1,"intValue":588448685,"type":"SIMPLE_B"}},{"op":"replace","path":"/collectionValue/9/@type","value":"SimpleReference"},{"op":"replace","path":"/collectionValue/9/id","value":"82141cc1-d655-4905-ba1a-a1a4d9070466"},{"op":"replace","path":"/collectionValue/9/notes","value":"interdum nonumes orci vero aptent suavitate"},{"op":"replace","path":"/collectionValue/9/description","value":"simul aenean morbi fames tempus veri reformidans"},{"op":"remove","path":"/collectionValue/9/intValue"},{"op":"replace","path":"/collectionValue/9/type","value":"SIMPLE_REFERENCE"},{"op":"add","path":"/collectionValue/9/referenceValue","value":{"@type":"SimpleB","id":"39958676-bfa2-4963-8c01-7a0e656fd805","notes":"numquam eloquentiam sagittis esse habemus offendit hac lectus sumo","description":"feugait interdum expetendis dicam vituperata quem nonumes","version":0,"intValue":1215734120,"type":"SIMPLE_B"}},{"op":"replace","path":"/collectionValue/10/@type","value":"SimpleA"},{"op":"replace","path":"/collectionValue/10/id","value":"12a3eea0-8636-4735-a4cb-03309eb14dad"},{"op":"replace","path":"/collectionValue/10/notes","value":"altera auctor civibus per tempor nonumes quaestio cetero"},{"op":"replace","path":"/collectionValue/10/description","value":"gravida sapientem saepe voluptatum aptent pri"},{"op":"remove","path":"/collectionValue/10/intValue"},{"op":"replace","path":"/collectionValue/10/type","value":"SIMPLE_A"},{"op":"move","path":"/collectionValue/10/stringValue","from":"/collectionValue/11/stringValue"},{"op":"add","path":"/collectionValue/12","value":{"@type":"SimpleD","id":"95a81a99-ce21-40c7-a152-8db57ea34a76","notes":"ancillae quisque eget ius praesent omittantur senserit nunc finibus","description":"ridens dolorum veritus neque hinc menandri netus","version":0,"booleanValue":false,"type":"SIMPLE_D"}}], + "node": {"@type":"SimpleCollection","id":"6b9c1dea-566c-4284-aab6-c590658fa02e","notes":"fabulas litora labores curabitur vestibulum finibus vehicula risus venenatis","description":"pertinacia conubia mi noster electram curabitur dicant dignissim","version":30,"collectionValue":[{"@type":"SimpleA","id":"12a3eea0-8636-4735-a4cb-03309eb14dad","notes":"altera auctor civibus per tempor nonumes quaestio cetero","description":"gravida sapientem saepe voluptatum aptent pri","version":0,"stringValue":"airline-fan-dream-just","type":"SIMPLE_A"},{"@type":"SimpleB","id":"e1011e30-9bd5-470e-a40a-4c3d7b621e16","notes":"liber vix elitr interpretaris aliquip dico quidam","description":"aenean adversarium vivamus id porta curabitur placerat invidunt","version":0,"intValue":273482951,"type":"SIMPLE_B"},{"@type":"SimpleB","id":"e1011e30-9bd5-470e-a40a-4c3d7b621e16","notes":"liber vix elitr interpretaris aliquip dico quidam","description":"aenean adversarium vivamus id porta curabitur placerat invidunt","version":0,"intValue":273482951,"type":"SIMPLE_B"},{"@type":"SimpleB","id":"e1011e30-9bd5-470e-a40a-4c3d7b621e16","notes":"liber vix elitr interpretaris aliquip dico quidam","description":"aenean adversarium vivamus id porta curabitur placerat invidunt","version":0,"intValue":273482951,"type":"SIMPLE_B"},{"@type":"SimpleB","id":"e1011e30-9bd5-470e-a40a-4c3d7b621e16","notes":"liber vix elitr interpretaris aliquip dico quidam","description":"aenean adversarium vivamus id porta curabitur placerat invidunt","version":0,"intValue":273482951,"type":"SIMPLE_B"},{"@type":"SimpleC","id":"e18f7bec-b247-4058-9a6c-c6b52af26388","notes":"option veri pellentesque ultricies adipiscing urbanitas disputationi sed consectetur","description":"pertinacia dicam odio ante eu utroque fames altera natoque","version":0,"doubleValue":0.07255739886557588,"type":"SIMPLE_C"},{"@type":"SimpleB","id":"e1011e30-9bd5-470e-a40a-4c3d7b621e16","notes":"liber vix elitr interpretaris aliquip dico quidam","description":"aenean adversarium vivamus id porta curabitur placerat invidunt","version":0,"intValue":273482951,"type":"SIMPLE_B"},{"@type":"SimpleC","id":"d4cfc023-1ad3-4af2-9d45-969eb1e18497","notes":"utroque ocurreret latine mediocritatem aenean porta recteque tristique","description":"quam fabulas molestiae pertinax elaboraret mei tempus veritus hinc","version":0,"doubleValue":0.16580470519942858,"type":"SIMPLE_C"},{"@type":"SimpleC","id":"d4cfc023-1ad3-4af2-9d45-969eb1e18497","notes":"utroque ocurreret latine mediocritatem aenean porta recteque tristique","description":"quam fabulas molestiae pertinax elaboraret mei tempus veritus hinc","version":0,"doubleValue":0.16580470519942858,"type":"SIMPLE_C"}],"type":"SIMPLE_COLLECTION"}, + "expected": "boom" } + ] } \ No newline at end of file From 2d652cb5389c0312a0a8bf016dfaa1b4576e0197 Mon Sep 17 00:00:00 2001 From: Tomer Gabel Date: Tue, 25 Jul 2017 16:37:20 +0300 Subject: [PATCH 10/14] Simplified and cleaned up failing test on #39, can now start fixing! --- src/test/resources/testdata/move.json | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/test/resources/testdata/move.json b/src/test/resources/testdata/move.json index 791c755..afdc8fc 100644 --- a/src/test/resources/testdata/move.json +++ b/src/test/resources/testdata/move.json @@ -15,6 +15,12 @@ "node": { "a": "b" }, "type": "InvalidJsonPatchException", "message": "missing 'from' field" + }, + { + "issue": 39, + "op": [{ "op": "move", "from": "/1/key", "path": "/0/key" }], + "node": [{ "key": "0" }], + "message": "noSuchPath" } ], "ops": [ @@ -42,13 +48,6 @@ "op": [{ "op": "move", "from": "/a", "path": "/b/2" }], "node": { "a": "helo", "b": [ 1, 2, 3, 4 ] }, "expected": { "b": [ 1, 2, "helo", 3, 4 ] } - }, - { - "issue": 39, - "op": [{"op":"replace","path":"/version","value":43},{"op":"move","path":"/collectionValue/0","from":"/collectionValue/1"},{"op":"add","path":"/collectionValue/1","value":{"@type":"SimpleReference","id":"9c144d02-a859-4a9c-be9f-0de6fc77d5ac","notes":"voluptatum moderatius dolorum finibus solet vituperata adhuc","description":"falli intellegat antiopam mus antiopam","version":0,"referenceValue":{"@type":"SimpleD","id":"c75d0410-9729-4688-94fc-d5416a2f716e","notes":"condimentum solum simul postea noluisse altera quam eruditi","description":"sem egestas senserit curabitur autem","version":0,"booleanValue":true,"type":"SIMPLE_D"},"type":"SIMPLE_REFERENCE"}},{"op":"add","path":"/collectionValue/4","value":{"@type":"SimpleB","id":"e1011e30-9bd5-470e-a40a-4c3d7b621e16","notes":"liber vix elitr interpretaris aliquip dico quidam","description":"aenean adversarium vivamus id porta curabitur placerat invidunt","version":1,"intValue":588448685,"type":"SIMPLE_B"}},{"op":"replace","path":"/collectionValue/9/@type","value":"SimpleReference"},{"op":"replace","path":"/collectionValue/9/id","value":"82141cc1-d655-4905-ba1a-a1a4d9070466"},{"op":"replace","path":"/collectionValue/9/notes","value":"interdum nonumes orci vero aptent suavitate"},{"op":"replace","path":"/collectionValue/9/description","value":"simul aenean morbi fames tempus veri reformidans"},{"op":"remove","path":"/collectionValue/9/intValue"},{"op":"replace","path":"/collectionValue/9/type","value":"SIMPLE_REFERENCE"},{"op":"add","path":"/collectionValue/9/referenceValue","value":{"@type":"SimpleB","id":"39958676-bfa2-4963-8c01-7a0e656fd805","notes":"numquam eloquentiam sagittis esse habemus offendit hac lectus sumo","description":"feugait interdum expetendis dicam vituperata quem nonumes","version":0,"intValue":1215734120,"type":"SIMPLE_B"}},{"op":"replace","path":"/collectionValue/10/@type","value":"SimpleA"},{"op":"replace","path":"/collectionValue/10/id","value":"12a3eea0-8636-4735-a4cb-03309eb14dad"},{"op":"replace","path":"/collectionValue/10/notes","value":"altera auctor civibus per tempor nonumes quaestio cetero"},{"op":"replace","path":"/collectionValue/10/description","value":"gravida sapientem saepe voluptatum aptent pri"},{"op":"remove","path":"/collectionValue/10/intValue"},{"op":"replace","path":"/collectionValue/10/type","value":"SIMPLE_A"},{"op":"move","path":"/collectionValue/10/stringValue","from":"/collectionValue/11/stringValue"},{"op":"add","path":"/collectionValue/12","value":{"@type":"SimpleD","id":"95a81a99-ce21-40c7-a152-8db57ea34a76","notes":"ancillae quisque eget ius praesent omittantur senserit nunc finibus","description":"ridens dolorum veritus neque hinc menandri netus","version":0,"booleanValue":false,"type":"SIMPLE_D"}}], - "node": {"@type":"SimpleCollection","id":"6b9c1dea-566c-4284-aab6-c590658fa02e","notes":"fabulas litora labores curabitur vestibulum finibus vehicula risus venenatis","description":"pertinacia conubia mi noster electram curabitur dicant dignissim","version":30,"collectionValue":[{"@type":"SimpleA","id":"12a3eea0-8636-4735-a4cb-03309eb14dad","notes":"altera auctor civibus per tempor nonumes quaestio cetero","description":"gravida sapientem saepe voluptatum aptent pri","version":0,"stringValue":"airline-fan-dream-just","type":"SIMPLE_A"},{"@type":"SimpleB","id":"e1011e30-9bd5-470e-a40a-4c3d7b621e16","notes":"liber vix elitr interpretaris aliquip dico quidam","description":"aenean adversarium vivamus id porta curabitur placerat invidunt","version":0,"intValue":273482951,"type":"SIMPLE_B"},{"@type":"SimpleB","id":"e1011e30-9bd5-470e-a40a-4c3d7b621e16","notes":"liber vix elitr interpretaris aliquip dico quidam","description":"aenean adversarium vivamus id porta curabitur placerat invidunt","version":0,"intValue":273482951,"type":"SIMPLE_B"},{"@type":"SimpleB","id":"e1011e30-9bd5-470e-a40a-4c3d7b621e16","notes":"liber vix elitr interpretaris aliquip dico quidam","description":"aenean adversarium vivamus id porta curabitur placerat invidunt","version":0,"intValue":273482951,"type":"SIMPLE_B"},{"@type":"SimpleB","id":"e1011e30-9bd5-470e-a40a-4c3d7b621e16","notes":"liber vix elitr interpretaris aliquip dico quidam","description":"aenean adversarium vivamus id porta curabitur placerat invidunt","version":0,"intValue":273482951,"type":"SIMPLE_B"},{"@type":"SimpleC","id":"e18f7bec-b247-4058-9a6c-c6b52af26388","notes":"option veri pellentesque ultricies adipiscing urbanitas disputationi sed consectetur","description":"pertinacia dicam odio ante eu utroque fames altera natoque","version":0,"doubleValue":0.07255739886557588,"type":"SIMPLE_C"},{"@type":"SimpleB","id":"e1011e30-9bd5-470e-a40a-4c3d7b621e16","notes":"liber vix elitr interpretaris aliquip dico quidam","description":"aenean adversarium vivamus id porta curabitur placerat invidunt","version":0,"intValue":273482951,"type":"SIMPLE_B"},{"@type":"SimpleC","id":"d4cfc023-1ad3-4af2-9d45-969eb1e18497","notes":"utroque ocurreret latine mediocritatem aenean porta recteque tristique","description":"quam fabulas molestiae pertinax elaboraret mei tempus veritus hinc","version":0,"doubleValue":0.16580470519942858,"type":"SIMPLE_C"},{"@type":"SimpleC","id":"d4cfc023-1ad3-4af2-9d45-969eb1e18497","notes":"utroque ocurreret latine mediocritatem aenean porta recteque tristique","description":"quam fabulas molestiae pertinax elaboraret mei tempus veritus hinc","version":0,"doubleValue":0.16580470519942858,"type":"SIMPLE_C"}],"type":"SIMPLE_COLLECTION"}, - "expected": "boom" } - ] } \ No newline at end of file From 04189ac073356678432c7b70086856bb976eae44 Mon Sep 17 00:00:00 2001 From: Tomer Gabel Date: Tue, 25 Jul 2017 16:38:46 +0300 Subject: [PATCH 11/14] Proper parsing of array indices, fixes 1/3 broken tests --- .../com/flipkart/zjsonpatch/InPlaceApplyProcessor.java | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/main/java/com/flipkart/zjsonpatch/InPlaceApplyProcessor.java b/src/main/java/com/flipkart/zjsonpatch/InPlaceApplyProcessor.java index 1c9ac9b..5b447e1 100644 --- a/src/main/java/com/flipkart/zjsonpatch/InPlaceApplyProcessor.java +++ b/src/main/java/com/flipkart/zjsonpatch/InPlaceApplyProcessor.java @@ -208,7 +208,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) { From a73263060c2bba7b09ab9c0503f4f5ca13f307d2 Mon Sep 17 00:00:00 2001 From: Tomer Gabel Date: Tue, 25 Jul 2017 18:35:38 +0300 Subject: [PATCH 12/14] Yet another error message improvement --- .../com/flipkart/zjsonpatch/AbstractTest.java | 33 ++++++++++++++++--- 1 file changed, 28 insertions(+), 5 deletions(-) diff --git a/src/test/java/com/flipkart/zjsonpatch/AbstractTest.java b/src/test/java/com/flipkart/zjsonpatch/AbstractTest.java index 150c608..060df98 100644 --- a/src/test/java/com/flipkart/zjsonpatch/AbstractTest.java +++ b/src/test/java/com/flipkart/zjsonpatch/AbstractTest.java @@ -19,14 +19,18 @@ 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.StringContains.containsString; 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; @@ -67,8 +71,21 @@ private Class exceptionType(String type) throws ClassNotFoundException { } private String errorMessage(String header) throws JsonProcessingException { - String testCase = new ObjectMapper().writerWithDefaultPrettyPrinter().writeValueAsString(p.getNode()); - return header + "\nFull test case (in file " + p.getSourceFile() + "):\n" + testCase; + 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 { @@ -85,10 +102,16 @@ private void testError() throws JsonProcessingException, ClassNotFoundException fail(errorMessage("Failure expected: " + message)); } catch (Exception e) { if (matchOnErrors()) { - assertThat(errorMessage("Operation failed but with wrong exception type"), e, instanceOf(type)); + 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"), + errorMessage("Operation failed but with wrong message", e), e.getMessage(), containsString(message.textValue())); // equalTo would be better, but fail existing tests } From d422d33bd0e11e2d6f67592c25e7597792d02b39 Mon Sep 17 00:00:00 2001 From: Tomer Gabel Date: Tue, 25 Jul 2017 18:36:05 +0300 Subject: [PATCH 13/14] Refactoring, cleanup and slightly more consistent messaging. 2/3 tests pass. --- .../zjsonpatch/InPlaceApplyProcessor.java | 152 ++++++++---------- 1 file changed, 71 insertions(+), 81 deletions(-) diff --git a/src/main/java/com/flipkart/zjsonpatch/InPlaceApplyProcessor.java b/src/main/java/com/flipkart/zjsonpatch/InPlaceApplyProcessor.java index 5b447e1..3844cf4 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,43 +55,39 @@ 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"); + 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 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 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"); - } + } 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("\"", ""); - if(!target.get(key).equals(value)){ - throw new JsonPatchApplicationException("[TEST Operation] value mismatch"); - } + } + else { + final ObjectNode target = (ObjectNode) parentNode; + String key = path.get(path.size() - 1).replaceAll("\"", ""); + if(!target.get(key).equals(value)){ + error(Operation.TEST, "value mismatch"); } } } @@ -100,22 +96,18 @@ else if (parentNode.isArray()) { @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 +133,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) { From 1b2dbb86f36b49a989334aeddfafc27907b67efc Mon Sep 17 00:00:00 2001 From: Tomer Gabel Date: Tue, 25 Jul 2017 18:39:21 +0300 Subject: [PATCH 14/14] All tests pass! Closes #39 --- .../java/com/flipkart/zjsonpatch/InPlaceApplyProcessor.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/main/java/com/flipkart/zjsonpatch/InPlaceApplyProcessor.java b/src/main/java/com/flipkart/zjsonpatch/InPlaceApplyProcessor.java index 3844cf4..f40a069 100644 --- a/src/main/java/com/flipkart/zjsonpatch/InPlaceApplyProcessor.java +++ b/src/main/java/com/flipkart/zjsonpatch/InPlaceApplyProcessor.java @@ -86,9 +86,11 @@ else if (parentNode.isArray()) { else { final ObjectNode target = (ObjectNode) parentNode; String key = path.get(path.size() - 1).replaceAll("\"", ""); - if(!target.get(key).equals(value)){ + 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"); - } } } }