From adba7d60dd8abd1cfa7bea20ed50ecbe95e7d292 Mon Sep 17 00:00:00 2001 From: Jonathan Knight Date: Wed, 23 Oct 2024 20:45:11 +0300 Subject: [PATCH] Cleaning up JsonBuilder.x and JsonMergePatch.x and adding tests --- lib_json/src/main/x/json/JsonBuilder.x | 38 +-- lib_json/src/main/x/json/JsonMergePatch.x | 75 ++++- .../main/x/json/json_test/JsonBuilderTest.x | 46 ++- .../x/json/json_test/JsonMergePatchTest.x | 316 +++++++++++++++++- 4 files changed, 435 insertions(+), 40 deletions(-) diff --git a/lib_json/src/main/x/json/JsonBuilder.x b/lib_json/src/main/x/json/JsonBuilder.x index 573490c676..8f0ea105c2 100644 --- a/lib_json/src/main/x/json/JsonBuilder.x +++ b/lib_json/src/main/x/json/JsonBuilder.x @@ -86,13 +86,14 @@ class JsonBuilder { Doc existing = get(id); switch (existing.is(_)) { case JsonObject: - mergeObjectMember(existing, path, doc, id); + mergeIntoObjectMember(existing, path, doc, id); break; case JsonArray: - mergeArrayMember(existing, path, doc, id); + mergeIntoArrayMember(existing, path, doc, id); break; case Primitive: - mergePrimitiveMember(existing, path, doc, id); + // existing is a primitive so it cannot be merged into and is instead replaced + replaceMember(path, doc, id); break; default: assert; @@ -113,14 +114,14 @@ class JsonBuilder { } /** - * Deeply merge a `JsonObject` value into this builder. + * Deeply merge a JSON value into a `JsonObject` in this builder. * - * @param obj the `JsonObject` to merge - * @param path the path to the location the object value should be merged into - * @param doc the value to merge the object into + * @param obj the `JsonObject` to merge into + * @param path the path to the location the `Doc` value should be merged into + * @param doc the JSON value to merge into the `JsonObject` * @param id the id of the entry being merged into */ - protected void mergeObjectMember(JsonObject obj, JsonPointer path, Doc doc, Id id) { + protected void mergeIntoObjectMember(JsonObject obj, JsonPointer path, Doc doc, Id id) { JsonPointer remainder = path.remainder ?: assert; JsonObject updated = new JsonObjectBuilder(obj).deepMerge(remainder, doc).build(); update(id, updated); @@ -139,28 +140,27 @@ class JsonBuilder { } /** - * Deeply merge a `JsonArray` value into this builder. + * Deeply merge a JSON value into a `JsonArray` value in this builder. * - * @param array the `JsonArray` to merge - * @param path the path to the location the array value should be merged into - * @param doc the value to merge the array into + * @param array the `JsonArray` to merge into + * @param path the path to the location the JSON value should be merged into + * @param doc the JSON value to merge the into the `JsonArray` * @param id the id of the entry being merged into */ - protected void mergeArrayMember(JsonArray array, JsonPointer path, Doc doc, Id id) { + protected void mergeIntoArrayMember(JsonArray array, JsonPointer path, Doc doc, Id id) { JsonPointer remainder = path.remainder ?: assert; JsonArray updated = new JsonArrayBuilder(array).deepMerge(remainder, doc).build(); update(id, updated); } /** - * Deeply merge a `Primitive` json value into this builder. + * Replace a value in this builder with a specified value. * - * @param obj the `Primitive` value to merge (TODO JK: value is not used!?) - * @param path the path to the location the primitive value should be merged into - * @param doc the value to merge the primitive into - * @param id the id of the entry being merged into + * @param path the path to the location of the value to replace + * @param doc the JSON value to replace any existing value with + * @param id the id of the entry being replaced */ - protected void mergePrimitiveMember(Primitive obj, JsonPointer path, Doc doc, Id id) { + protected void replaceMember(JsonPointer path, Doc doc, Id id) { JsonPointer remainder = path.remainder ?: assert; JsonObject updated = new JsonObjectBuilder().deepMerge(remainder, doc).build(); update(id, updated); diff --git a/lib_json/src/main/x/json/JsonMergePatch.x b/lib_json/src/main/x/json/JsonMergePatch.x index 3e14c52cd8..726ad58597 100644 --- a/lib_json/src/main/x/json/JsonMergePatch.x +++ b/lib_json/src/main/x/json/JsonMergePatch.x @@ -30,33 +30,80 @@ class JsonMergePatch(Doc patch) { return merge(target, patch, inPlace); } + /** + * Perform a merge as described by the pseudo code in RFC 7396. + * + * define MergePatch(Target, Patch): + * if Patch is an Object: + * if Target is not an Object: + * Target = {} # Ignore the contents and set it to an empty Object + * for each Name/Value pair in Patch: + * if Value is null: + * if Name exists in Target: + * remove the Name/Value pair from Target + * else: + * Target[Name] = MergePatch(Target[Name], Value) + * return Target + * else: + * return Patch + * + * * If the `patch` parameter is not a `JsonObject` the `patch` parameter is returned as the result. + * + * * If the target `Doc` is not a `JsonObject` it is ignored and the merge will be applied to + * a new empty `JsonObject`. + * + * * If the target `Doc` is a mutable `JsonObject` and the `inPlace` parameter is `True` the merge will be + * applied directly to the target. + * + * * A `Null` value for a key in the `patch` will cause the corresponding entry in the target to be removed. + * Any `Null` value in the `patch` will not appear in the merged result. + * + * @param doc that target JSON value to apply the patch to + * @param patch the JSON value representing the patch to apply + * @param inPlace (optional) `True` to modify the target in place (if applicable), or `False` + * to leave the target unmodified and return a patched copy of the target + * + * @return the JSON value resulting from applying this patch to the target + */ private Doc merge(Doc doc, Doc patch, Boolean inPlace = False) { if (patch.is(JsonObject)) { JsonObject target; - if (doc.is(JsonObject)) { - target = doc; + if (doc.is(immutable) || !inPlace) { + // we can make in place true as we are making a new target so there is + // no point continually copying target elements from here on + inPlace = True; + target = json.newObject(); + target.putAll(doc); + } else { + target = doc; + } } else { - target = json.newObject(); + // we can make in place true as we are making a new target so there is + // no point continually copying target elements from here on + inPlace = True; + target = json.newObject(); } - JsonObjectBuilder builder = new JsonObjectBuilder(target); - for (Map.Entry entry : patch.entries) { - String key = entry.key; - Doc value = entry.value; + for ((String key, Doc value) : patch) { if (value == Null) { target.remove(key); } else { - if (Doc targetValue := target.get(key)) { - merge(key, merge(targetValue, value, inPlace)); - } else { - // merging the value into a new JsonObject (hence inPlace is `True`) - merge(key, merge(json.newObject(), value, True)); - } + target[key] = merge(target[key], value, inPlace); } } - return builder.build(); + // TODO JK: + // If the original target is immutable the target being returned will be a copy + // that is currently mutable. Should it be made immutable to match the original + // target doc parameter? + // Basically, should the mutability of the result match the mutability of the + // original doc parameter? + return target; } + // TODO JK: + // Should a copy of the patch be returned and should it be immutable? + // If we do make a copy, should the mutability of the result match the mutability of the + // original doc parameter? return patch; } diff --git a/manualTests/src/main/x/json/json_test/JsonBuilderTest.x b/manualTests/src/main/x/json/json_test/JsonBuilderTest.x index 120ca7239c..6295c670e1 100644 --- a/manualTests/src/main/x/json/json_test/JsonBuilderTest.x +++ b/manualTests/src/main/x/json/json_test/JsonBuilderTest.x @@ -76,9 +76,9 @@ class JsonBuilderTest { void assertDeepCopyObject(JsonObject source, JsonObject copy) { assert © != &source as "source and copy should be different object references"; assert copy.inPlace == True as "copy should be mutable"; - for (Map.Entry entry : source.entries) { - assert Doc copyValue := copy.get(entry.key); - assertDeepCopy(entry.value, copyValue); + for ((String key, Doc value) : source) { + assert Doc copyValue := copy.get(key); + assertDeepCopy(value, copyValue); } } @@ -142,10 +142,12 @@ class JsonBuilderTest { JsonObject objExpected1 = Map:["three"=3, "four"=44, "five"=5]; JsonObject source = Map:["c"=Map:["1"=objUpdate1]]; JsonObject result = new JsonObjectBuilder(target).deepMerge(source).build(); + Doc a = result["a"]; Doc c = result["c"]; + assert a.is(String); + assert a == "b"; assert c.is(Array); assert c == Array:[objOrig0, objExpected1, objOrig2]; - //assert result == Map:["a"="b", "c"=Array:[objOrig0, objExpected1, objOrig2]]; } @Test @@ -178,11 +180,45 @@ class JsonBuilderTest { assert target == Map:["a"="b", "c"=Array:["one", "two", "three"]]; } + // ---- Merge into primitive fields ------------------------------------------------------------ + @Test - void shouldMergeObjectWithPrimitiveIntoObjectWithExistingPrimitive() { + void shouldMergePrimitiveIntoObjectWithExistingPrimitiveField() { JsonObject target = Map:["a"="b", "c"="d"]; JsonObject source = Map:["c"="updated"]; JsonObject result = new JsonObjectBuilder(target).deepMerge(source).build(); assert result == Map:["a"="b", "c"="updated"]; } + + @Test + void shouldMergePrimitiveIntoObjectWithoutExistingPrimitiveField() { + JsonObject target = Map:["a"="b"]; + JsonObject source = Map:["c"="d"]; + JsonObject result = new JsonObjectBuilder(target).deepMerge(source).build(); + assert result == Map:["a"="b", "c"="d"]; + } + + @Test + void shouldMergeObjectIntoObjectWithExistingPrimitiveField() { + JsonObject child = Map:["child-one"="value-one", "child-two"="value-two"]; + JsonObject target = Map:["a"="b", "c"="d"]; + JsonObject source = Map:["c"=child]; + JsonObject result = new JsonObjectBuilder(target).deepMerge(source).build(); + assert result["a"] == "b"; + Doc c = result["c"]; + assert c.is(JsonObject); + assert c == child; + } + + @Test + void shouldMergeArrayIntoObjectWithExistingPrimitiveField() { + JsonArray child = Array:[1, 2, 3]; + JsonObject target = Map:["a"="b", "c"="d"]; + JsonObject source = Map:["c"=child]; + JsonObject result = new JsonObjectBuilder(target).deepMerge(source).build(); + assert result["a"] == "b"; + Doc c = result["c"]; + assert c.is(JsonArray); + assert c == child; + } } \ No newline at end of file diff --git a/manualTests/src/main/x/json/json_test/JsonMergePatchTest.x b/manualTests/src/main/x/json/json_test/JsonMergePatchTest.x index 08376a955d..72976b56b0 100644 --- a/manualTests/src/main/x/json/json_test/JsonMergePatchTest.x +++ b/manualTests/src/main/x/json/json_test/JsonMergePatchTest.x @@ -1,14 +1,326 @@ +import json.Doc; +import json.JsonArray; import json.JsonMergePatch; import json.JsonObject; +import json.Primitive; class JsonMergePatchTest { @Test - void shouldBeEmpty() { + void shouldCreateAnEmptyPatch() { JsonObject o = json.newObject(); JsonMergePatch patch = new JsonMergePatch(o); assert patch.empty; } - + /** + * Test from the examples in Appendix A of RFC 7396 + * + * ORIGINAL PATCH RESULT + * ------------------------------------------ + * {"a":"b"} {"a":"c"} {"a":"c"} + */ + @Test + void shouldMergeReplacingSingleKeyValue() { + JsonObject original = Map:["a"="b"]; + JsonMergePatch patch = new JsonMergePatch(Map:["a"="c"]); + Doc result = patch.apply(original); + assert result.is(JsonObject); + assert result == Map:["a"="c"]; + } + + /** + * Test from the examples in Appendix A of RFC 7396 + * + * ORIGINAL PATCH RESULT + * ------------------------------------------ + * {"a":"b"} {"b":"c"} {"a":"b", + * "b":"c"} + */ + @Test + void shouldMergeAddingNewEntry() { + JsonObject original = Map:["a"="b"]; + JsonMergePatch patch = new JsonMergePatch(Map:["b"="c"]); + Doc result = patch.apply(original); + assert result.is(JsonObject); + assert result == Map:["a"="b", "b"="c"]; + } + + /** + * Test from the examples in Appendix A of RFC 7396 + * + * ORIGINAL PATCH RESULT + * ------------------------------------------ + * {"a":"b"} {"a":null} {} + */ + @Test + void shouldMergeRemovingOnlyEntry() { + JsonObject original = Map:["a"="b"]; + JsonMergePatch patch = new JsonMergePatch(Map:["a"=Null]); + Doc result = patch.apply(original); + assert result.is(JsonObject); + assert result.empty; + } + + /** + * Test from the examples in Appendix A of RFC 7396 + * + * ORIGINAL PATCH RESULT + * ------------------------------------------ + * {"a":"b", {"a":null} {"b":"c"} + * "b":"c"} + */ + @Test + void shouldMergeRemovingEntry() { + JsonObject original = Map:["a"="b", "b"="c"]; + JsonMergePatch patch = new JsonMergePatch(Map:["a"=Null]); + Doc result = patch.apply(original); + assert result.is(JsonObject); + assert result == Map:["b"="c"]; + } + + /** + * Test from the examples in Appendix A of RFC 7396 + * + * ORIGINAL PATCH RESULT + * ------------------------------------------ + * {"a":["b"]} {"a":"c"} {"a":"c"} + */ + @Test + void shouldMergeReplacingArrayWithPrimitive() { + JsonArray array = Array:["b"]; + JsonObject original = Map:["a"=array]; + JsonMergePatch patch = new JsonMergePatch(Map:["a"="c"]); + Doc result = patch.apply(original); + assert result.is(JsonObject); + assert result == Map:["a"="c"]; + } + + /** + * Test from the examples in Appendix A of RFC 7396 + * + * ORIGINAL PATCH RESULT + * ------------------------------------------ + * {"a":"c"} {"a":["b"]} {"a":["b"]} + */ + @Test + void shouldMergeReplacingPrimitiveWithArray() { + JsonObject original = Map:["a"="c"]; + JsonArray array = Array:["b"]; + JsonMergePatch patch = new JsonMergePatch(Map:["a"=array]); + Doc result = patch.apply(original); + assert result.is(JsonObject); + assert result == Map:["a"=array]; + } + + /** + * Test from the examples in Appendix A of RFC 7396 + * + * ORIGINAL PATCH RESULT + * ------------------------------------------ + * {"a": { {"a": { {"a": { + * "b": "c"} "b": "d", "b": "d" + * } "c": null} } + * } } + */ + @Test + void shouldMergeReplacingChildEntryIgnoringNullValue() { + JsonObject original = Map:["a"=Map:["b"="c"]]; + JsonMergePatch patch = new JsonMergePatch(Map:["a"=Map:["b"="d", "c"=Null]]); + Doc result = patch.apply(original); + assert result.is(JsonObject); + assert result == Map:["a"=Map:["b"="d"]]; + } + + /** + * Test from the examples in Appendix A of RFC 7396 + * + * ORIGINAL PATCH RESULT + * ------------------------------------------ + * {"a": [ {"a": [1]} {"a": [1]} + * {"b":"c"} + * ] + * } + */ + @Test + void shouldMergeReplacingChildObjectWithArray() { + JsonObject original = Map:["a"=Map:["b"="c"]]; + JsonArray array = Array:[1]; + JsonMergePatch patch = new JsonMergePatch(Map:["a"=array]); + Doc result = patch.apply(original); + assert result.is(JsonObject); + assert result == Map:["a"=array]; + } + + /** + * Test from the examples in Appendix A of RFC 7396 + * + * ORIGINAL PATCH RESULT + * ------------------------------------------ + * ["a","b"] ["c","d"] ["c","d"] + */ + @Test + void shouldMergeWhereTargetAndPatchAreArrays() { + JsonArray original = Array:["a", "b"]; + JsonMergePatch patch = new JsonMergePatch(Array:["c", "d"]); + Doc result = patch.apply(original); + assert result.is(JsonArray); + assert result == Array:["c", "d"]; + } + + @Test + void shouldMergeWhereTargetAndPatchArePrimitives() { + Doc original = "a"; + JsonMergePatch patch = new JsonMergePatch("b"); + Doc result = patch.apply(original); + assert result.is(Primitive); + assert result == "b"; + } + + /** + * Test from the examples in Appendix A of RFC 7396 + * + * ORIGINAL PATCH RESULT + * ------------------------------------------ + * {"a":"b"} ["c"] ["c"] + */ + @Test + void shouldMergeWherePatchIsArray() { + JsonObject original = Map:["a"="b"]; + JsonMergePatch patch = new JsonMergePatch(Array:["c"]); + Doc result = patch.apply(original); + assert result.is(JsonArray); + assert result == Array:["c"]; + } + + /** + * Test from the examples in Appendix A of RFC 7396 + * + * ORIGINAL PATCH RESULT + * ------------------------------------------ + * {"a":"foo"} null null + */ + @Test + void shouldMergeWherePatchIsNull() { + JsonObject original = Map:["a"="b"]; + JsonMergePatch patch = new JsonMergePatch(Null); + Doc result = patch.apply(original); + assert result.is(Primitive); + assert result == Null; + } + + /** + * Test from the examples in Appendix A of RFC 7396 + * + * ORIGINAL PATCH RESULT + * ------------------------------------------ + * {"a":"foo"} "bar" "bar" + */ + @Test + void shouldMergeWherePatchIsPrimitive() { + JsonObject original = Map:["a"="b"]; + JsonMergePatch patch = new JsonMergePatch("bar"); + Doc result = patch.apply(original); + assert result.is(Primitive); + assert result == "bar"; + } + + /** + * Test from the examples in Appendix A of RFC 7396 + * + * ORIGINAL PATCH RESULT + * ------------------------------------------ + * {"e":null} {"a":1} {"e":null, + * "a":1} + */ + @Test + void shouldMergeAddingNewEntryWhereTargetContainsNull() { + JsonObject original = Map:["e"=Null]; + JsonMergePatch patch = new JsonMergePatch(Map:["a"=1]); + Doc result = patch.apply(original); + assert result.is(JsonObject); + assert result == Map:["e"=Null, "a"=1]; + } + + /** + * Test from the examples in Appendix A of RFC 7396 + * + * ORIGINAL PATCH RESULT + * ------------------------------------------ + * [1,2] {"a":"b", {"a":"b"} + * "c":null} + */ + @Test + void shouldMergeWhereTargetIsArrayIgnoringNullValuesInPatch() { + JsonArray original = Array:[1, 2]; + JsonMergePatch patch = new JsonMergePatch(Map:["a"="b", "c"=Null]); + Doc result = patch.apply(original); + assert result.is(JsonObject); + assert result == Map:["a"="b"]; + } + + /** + * Test from the examples in Appendix A of RFC 7396 + * + * ORIGINAL PATCH RESULT + * ------------------------------------------ + * {} {"a": {"a": + * {"bb": {"bb": + * {"ccc": {}}} + * null}}} + */ + @Test + void shouldMergeWhereTargetIsEmptyIgnoringNullValuesInPatch() { + JsonObject original = Map:[]; + JsonMergePatch patch = new JsonMergePatch(Map:["a"=Map:["bb"=Map:["ccc"=Null]]]); + Doc result = patch.apply(original); + assert result.is(JsonObject); + assert result.size == 1; + Doc a = result["a"]; + assert a.is(JsonObject); + assert a.size == 1; + Doc bb = a["bb"]; + assert bb.is(JsonObject); + assert bb.empty; + } + + @Test + void shouldMergeInPlaceWhenTargetIsMutableAndInPlaceIsTrue() { + JsonObject original = json.newObject(); + JsonMergePatch patch = new JsonMergePatch(Map:["a"="b"]); + Doc result = patch.apply(original, True); + assert &result == &original as "source and copy should be the same object reference"; + assert result.is(JsonObject); + assert result == Map:["a"="b"]; + } + + @Test + void shouldMergeMakingCopyWhenTargetIsImmutableAndInPlaceIsTrue() { + JsonObject original = Map:[]; + JsonMergePatch patch = new JsonMergePatch(Map:["a"="b"]); + Doc result = patch.apply(original, True); + assert &result != &original as "source and copy should not be the same object reference"; + assert result.is(JsonObject); + assert result == Map:["a"="b"]; + } + + @Test + void shouldMergeMakingCopyWhenTargetIsMutableAndInPlaceIsFalse() { + JsonObject original = new HashMap(); + JsonMergePatch patch = new JsonMergePatch(Map:["a"="b"]); + Doc result = patch.apply(original, False); + assert &result != &original as "source and copy should not be the same object reference"; + assert result.is(JsonObject); + assert result == Map:["a"="b"]; + } + + @Test + void shouldMergeMakingCopyWhenTargetIsMutableAndInPlaceNotSpecified() { + JsonObject original = new HashMap(); + JsonMergePatch patch = new JsonMergePatch(Map:["a"="b"]); + Doc result = patch.apply(original); + assert &result != &original as "source and copy should not be the same object reference"; + assert result.is(JsonObject); + assert result == Map:["a"="b"]; + } } \ No newline at end of file