Cleaning up JsonBuilder.x and JsonMergePatch.x and adding tests

lib_json/src/main/x/json/JsonBuilder.x

@@ -86,13 +86,14 @@ class JsonBuilder<JsonType extends JsonStruct, Id extends Int | String> {
             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<JsonType extends JsonStruct, Id extends Int | String> {
     }
 
     /**
-     * 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<JsonType extends JsonStruct, Id extends Int | String> {
     }
 
     /**
-     * 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);

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<String, Doc> 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;
     }
 

manualTests/src/main/x/json/json_test/JsonBuilderTest.x

@@ -76,9 +76,9 @@ class JsonBuilderTest {
     void assertDeepCopyObject(JsonObject source, JsonObject copy) {
         assert &copy != &source as "source and copy should be different object references";
         assert copy.inPlace == True as "copy should be mutable";
-        for (Map.Entry<String, Doc> 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<String, Doc>:["three"=3, "four"=44, "five"=5];
         JsonObject source       = Map<String, Doc>:["c"=Map<String, Doc>:["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<Doc>);
         assert c == Array<Doc>:[objOrig0, objExpected1, objOrig2];
-        //assert result == Map<String, Doc>:["a"="b", "c"=Array<Doc>:[objOrig0, objExpected1, objOrig2]];
     }
 
     @Test
@@ -178,11 +180,45 @@ class JsonBuilderTest {
         assert target == Map<String, Doc>:["a"="b", "c"=Array<Doc>:["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<Doc>:[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;
+    }
 }