Fix json_patch_apply handling of removing the whole document (i.e. "path":"").

Enable all disabled tests, add a few more including some with null documents.

diff --git a/json_patch.c b/json_patch.c
index dbc54ecc076112c7156c1bd2fa8051cda3e1513f..5bbc308a99d208763e6d6d51344883c854215727 100644 (file)
--- a/json_patch.c
+++ b/json_patch.c
@@ -81,7 +81,10 @@ static int __json_patch_apply_remove(struct json_pointer_get_result *jpres)
                json_object_object_del(jpres->parent, jpres->key_in_parent);
                return 0;
        } else {
-               return json_object_put(jpres->obj);
+               // We're removing the root object
+               (void)json_object_put(jpres->obj);
+               jpres->obj = NULL;
+               return 0;
        }
 }
 
@@ -99,6 +102,9 @@ static int json_patch_apply_remove(struct json_object **res, const char *path, s
        rc = __json_patch_apply_remove(&jpres);
        if (rc < 0)
                _set_err(EINVAL, "Unable to remove path referenced by 'path' field");
+       // This means we removed and freed the root object, i.e. *res
+       if (jpres.parent == NULL)
+               *res = NULL;
        return rc;
 }
 
@@ -223,6 +229,8 @@ static int json_patch_apply_move_copy(struct json_object **res,
                return rc;
        }
 
+       // Note: it's impossible for json_pointer to find the root obj, due
+       // to the path check above, so from.parent is guaranteed non-NULL
        json_object_get(from.obj);
 
        if (!move) {

diff --git a/tests/json_patch_spec_tests.json b/tests/json_patch_spec_tests.json
index c160535b340f7c21388a463616ef0bfb4db5f9ca..d7d407d3f5e3a7449090aa3b9149a071c2392616 100644 (file)
--- a/tests/json_patch_spec_tests.json
+++ b/tests/json_patch_spec_tests.json
@@ -192,8 +192,8 @@
     "patch": [
   { "op": "add", "path": "/baz", "value": "qux", "op": "remove" }
 ],
-    "error": "operation has two 'op' members",
-    "disabled": true
+    "error_wont_happen_in_jsonc": "operation has two 'op' members",
+    "error": "Did not find element referenced by path field"
   },
 
   {

diff --git a/tests/json_patch_tests.json b/tests/json_patch_tests.json
index 4124d513d2f25cd5b9343084f56cd0efe9b45836..8cafb93c961ec6028f173a15e7919185203779e3 100644 (file)
--- a/tests/json_patch_tests.json
+++ b/tests/json_patch_tests.json
@@ -52,8 +52,8 @@
     { "comment": "Toplevel scalar values OK?",
       "doc": "foo",
       "patch": [{"op": "replace", "path": "", "value": "bar"}],
-      "expected": "bar",
-      "disabled": true },
+      "expected": "bar"
+      },
 
     { "comment": "replace object document with array document?",
       "doc": {},
@@ -202,6 +202,55 @@
       "patch": [{"op": "replace", "path": "", "value": {"baz": "qux"}}],
       "expected": {"baz": "qux"} },
 
+    { "comment": "add whole document, null",
+      "doc": {},
+      "Note1": "We can't pass null in to json_patch_apply, so start with _something_ and remove it",
+      "patch": [
+        {"op": "remove", "path": ""},
+        {"op": "add", "path": "", "value": {"baz": "qux"}}
+      ],
+      "expected": {"baz": "qux"} },
+
+    { "comment": "replace whole document, null",
+      "doc": {},
+      "Note1": "We can't pass null in to json_patch_apply, so start with _something_ and remove it",
+      "patch": [
+        {"op": "remove", "path": ""},
+        {"op": "replace", "path": "", "value": {"baz": "qux"}}
+      ],
+      "error": "The spec says the target location must exist, so replacing a null document fails"
+    },
+
+    { "comment": "remove whole document",
+      "doc": {"foo": "bar"},
+      "patch": [{"op": "remove", "path": ""}],
+      "expected": null },
+
+    { "comment": "remove whole document",
+      "doc": {"foo": "bar"},
+      "patch": [{"op": "remove", "path": ""}],
+      "expected": null },
+
+    { "comment": "remove whole document, array",
+      "doc": ["foo", "bar"],
+      "patch": [{"op": "remove", "path": ""}],
+      "expected": null },
+
+    { "comment": "remove whole document, string",
+      "doc": "foo",
+      "patch": [{"op": "remove", "path": ""}],
+      "expected": null },
+
+    { "comment": "remove whole document, null",
+      "doc": {},
+      "Note1": "We can't pass null in to json_patch_apply, so start with _something_ and remove it",
+      "patch": [
+          {"op": "remove", "path": ""},
+          {"op": "remove", "path": ""},
+      ],
+      "error": "The spec says the target location must exist, so removing a null document fails"
+    },
+
     { "comment": "test replace with missing parent key should fail",
       "doc": {"bar": "baz"},
       "patch": [{"op": "replace", "path": "/foo/bar", "value": false}],
@@ -261,10 +310,16 @@
       "patch": [{"op": "test", "path": "/foo", "value": [1, 2]}],
       "error": "test op should fail" },
 
-    { "comment": "Whole document",
+    { "comment": "Test the whole document",
       "doc": { "foo": 1 },
       "patch": [{"op": "test", "path": "", "value": {"foo": 1}}],
-      "disabled": true },
+      "expected": { "foo": 1 } },
+
+    { "comment": "Test the whole document, no match",
+      "doc": { "foo": 1 },
+      "patch": [{"op": "test", "path": "", "value": {"foo": 2}}],
+      "expected": { "foo": 1 },
+      "error": "Tested value does not match original doc" },
 
     { "comment": "Empty-string element",
       "doc": { "": 1 },
@@ -438,13 +493,13 @@
       "patch": [ { "op": "move", "from": "/bar", "path": "/foo" } ],
       "error": "missing 'from' location" },
 
-    { "comment": "duplicate ops",
+    { "comment": "duplicate ops, json-c parses this as op:move",
       "doc": { "foo": "bar" },
       "patch": [ { "op": "add", "path": "/baz", "value": "qux",
                    "op": "move", "from":"/foo" } ],
-      "error": "patch has two 'op' members",
-      "disabled_in_json_c": true,
-      "disabled": true },
+      "error_wont_happen_in_jsonc": "patch has two 'op' members",
+      "expected": { "baz": "bar" }
+      },
 
     { "comment": "unrecognized op should fail",
       "doc": {"foo": 1},

diff --git a/tests/test_json_patch.c b/tests/test_json_patch.c
index d53c980c03100f4b0a92f6a92b768958ed291a16..eeb791c95e2eb215109e994b6fe099eebb1b1091 100644 (file)
--- a/tests/test_json_patch.c
+++ b/tests/test_json_patch.c
@@ -21,9 +21,9 @@ void test_json_patch_op(struct json_object *jo)
        const char *comment = json_object_get_string(json_object_object_get(jo, "comment"));
        struct json_object *doc = json_object_object_get(jo, "doc");
        struct json_object *patch = json_object_object_get(jo, "patch");
-       struct json_object *expected = json_object_object_get(jo, "expected");
+       struct json_object *expected = NULL;
+       json_bool have_expected = json_object_object_get_ex(jo, "expected", &expected);
        struct json_object *error = json_object_object_get(jo, "error");
-       int disabled_test = json_object_get_boolean(json_object_object_get(jo, "disabled_in_json_c"));
        const char *error_s = json_object_get_string(error);
        struct json_object *res = NULL;
        int ret;
@@ -32,13 +32,9 @@ void test_json_patch_op(struct json_object *jo)
                comment ? comment : error_s,
                json_object_get_string(doc),
                json_object_get_string(patch));
-       if (disabled_test) {
-               printf("SKIPPING - disabled in the test spec\n");
-               return;
-       }
-       if (!error && !expected) {
-               printf("SKIPPING - no expected or error conditions in test\n");
-               return;
+       if (!error && !have_expected) {
+               printf("BAD TEST - no expected or error conditions in test: %s\n", json_object_to_json_string(jo));
+               assert(0);
        }
        fflush(stdout);
        struct json_patch_error jperr;
@@ -54,13 +50,13 @@ void test_json_patch_op(struct json_object *jo)
                if (ret) {
                        fprintf(stderr, "json_patch_apply() returned '%d'\n", ret);
                        fprintf(stderr, "Expected: %s\n", json_object_get_string(expected));
-                       fprintf(stderr, "Got: %s\n", json_object_get_string(res));
+                       fprintf(stderr, "Got: %s\n", res ? json_object_get_string(res) : "(null)");
                        fprintf(stderr, "json_patch_apply failed: %s at patch idx %zu: %s\n",
                                strerror(jperr.errno_code), jperr.patch_failure_idx, jperr.errmsg);
                        fflush(stderr);
                        assert(0);
                }
-               assert(res != NULL);
+               // Note: res might be NULL if the whole document was removed
                assert(jperr.errno_code == 0);
                ret = json_object_equal(expected, res);
                if (ret == 0) {
@@ -83,6 +79,7 @@ void test_json_patch_using_file(const char *testdir, const char *filename)
        (void)snprintf(full_filename, sizeof(full_filename), "%s/%s", testdir, filename);
        size_t ii;
 
+       printf("Testing using file %s\n", filename);
        json_object *jo = json_object_from_file(full_filename);
        if (!jo) {
                fprintf(stderr, "FAIL: unable to open %s: %s\n", full_filename, strerror(errno));

diff --git a/tests/test_json_patch.expected b/tests/test_json_patch.expected
index 9d2887a4a38a67063a70631ee959fa007f24ded1..523d276b25def73e624a88cdef00bb31b9110cd6 100644 (file)
--- a/tests/test_json_patch.expected
+++ b/tests/test_json_patch.expected
@@ -1,3 +1,4 @@
+Testing using file json_patch_spec_tests.json
 Testing '4.1. add with missing object', doc '{ "q": { "bar": 2 } }' patch '[ { "op": "add", "path": "\/a\/b", "value": 1 } ]' : OK
  => json_patch_apply failed as expected: ERRNO=ENOENT at patch idx 0: Failed to set value at path referenced by 'path' field
 Testing 'A.1.  Adding an Object Member', doc '{ "foo": "bar" }' patch '[ { "op": "add", "path": "\/baz", "value": "qux" } ]' : OK
@@ -20,6 +21,7 @@ Testing 'A.14. ~ Escape Ordering', doc '{ "\/": 9, "~1": 10 }' patch '[ { "op":
 Testing 'A.15. Comparing Strings and Numbers', doc '{ "\/": 9, "~1": 10 }' patch '[ { "op": "test", "path": "\/~01", "value": "10" } ]' : OK
  => json_patch_apply failed as expected: ERRNO=ENOENT at patch idx 0: Value of element referenced by 'path' field did not match 'value' field
 Testing 'A.16. Adding an Array Value', doc '{ "foo": [ "bar" ] }' patch '[ { "op": "add", "path": "\/foo\/-", "value": [ "abc", "def" ] } ]' : OK
+Testing using file json_patch_tests.json
 Testing 'empty list, empty docs', doc '{ }' patch '[ ]' : OK
 Testing 'empty patch list', doc '{ "foo": 1 }' patch '[ ]' : OK
 Testing 'rearrangements OK?', doc '{ "foo": 1, "bar": 2 }' patch '[ ]' : OK
@@ -69,6 +71,15 @@ Testing '(null)', doc '[ "" ]' patch '[ { "op": "replace", "path": "\/0", "value
 Testing '(null)', doc '[ "" ]' patch '[ { "op": "replace", "path": "\/0", "value": null } ]' : OK
 Testing 'value in array replace not flattened', doc '[ "foo", "sil" ]' patch '[ { "op": "replace", "path": "\/1", "value": [ "bar", "baz" ] } ]' : OK
 Testing 'replace whole document', doc '{ "foo": "bar" }' patch '[ { "op": "replace", "path": "", "value": { "baz": "qux" } } ]' : OK
+Testing 'add whole document, null', doc '{ }' patch '[ { "op": "remove", "path": "" }, { "op": "add", "path": "", "value": { "baz": "qux" } } ]' : OK
+Testing 'replace whole document, null', doc '{ }' patch '[ { "op": "remove", "path": "" }, { "op": "replace", "path": "", "value": { "baz": "qux" } } ]' : OK
+ => json_patch_apply failed as expected: ERRNO=EINVAL at patch idx 1: Invalid path field
+Testing 'remove whole document', doc '{ "foo": "bar" }' patch '[ { "op": "remove", "path": "" } ]' : OK
+Testing 'remove whole document', doc '{ "foo": "bar" }' patch '[ { "op": "remove", "path": "" } ]' : OK
+Testing 'remove whole document, array', doc '[ "foo", "bar" ]' patch '[ { "op": "remove", "path": "" } ]' : OK
+Testing 'remove whole document, string', doc 'foo' patch '[ { "op": "remove", "path": "" } ]' : OK
+Testing 'remove whole document, null', doc '{ }' patch '[ { "op": "remove", "path": "" }, { "op": "remove", "path": "" } ]' : OK
+ => json_patch_apply failed as expected: ERRNO=EINVAL at patch idx 1: Invalid path field
 Testing 'test replace with missing parent key should fail', doc '{ "bar": "baz" }' patch '[ { "op": "replace", "path": "\/foo\/bar", "value": false } ]' : OK
  => json_patch_apply failed as expected: ERRNO=ENOENT at patch idx 0: Did not find element referenced by path field
 Testing 'spurious patch properties', doc '{ "foo": 1 }' patch '[ { "op": "test", "path": "\/foo", "value": 1, "spurious": 1 } ]' : OK
@@ -83,7 +94,9 @@ Testing 'test should pass despite (nested) rearrangement', doc '{ "foo": [ { "fo
 Testing 'test should pass - no error', doc '{ "foo": { "bar": [ 1, 2, 5, 4 ] } }' patch '[ { "op": "test", "path": "\/foo", "value": { "bar": [ 1, 2, 5, 4 ] } } ]' : OK
 Testing 'test op should fail', doc '{ "foo": { "bar": [ 1, 2, 5, 4 ] } }' patch '[ { "op": "test", "path": "\/foo", "value": [ 1, 2 ] } ]' : OK
  => json_patch_apply failed as expected: ERRNO=ENOENT at patch idx 0: Value of element referenced by 'path' field did not match 'value' field
-Testing 'Whole document', doc '{ "foo": 1 }' patch '[ { "op": "test", "path": "", "value": { "foo": 1 } } ]' : SKIPPING - no expected or error conditions in test
+Testing 'Test the whole document', doc '{ "foo": 1 }' patch '[ { "op": "test", "path": "", "value": { "foo": 1 } } ]' : OK
+Testing 'Test the whole document, no match', doc '{ "foo": 1 }' patch '[ { "op": "test", "path": "", "value": { "foo": 2 } } ]' : OK
+ => json_patch_apply failed as expected: ERRNO=ENOENT at patch idx 0: Value of element referenced by 'path' field did not match 'value' field
 Testing 'Empty-string element', doc '{ "": 1 }' patch '[ { "op": "test", "path": "\/", "value": 1 } ]' : OK
 Testing '(null)', doc '{ "foo": [ "bar", "baz" ], "": 0, "a\/b": 1, "c%d": 2, "e^f": 3, "g|h": 4, "i\\j": 5, "k\"l": 6, " ": 7, "m~n": 8 }' patch '[ { "op": "test", "path": "\/foo", "value": [ "bar", "baz" ] }, { "op": "test", "path": "\/foo\/0", "value": "bar" }, { "op": "test", "path": "\/", "value": 0 }, { "op": "test", "path": "\/a~1b", "value": 1 }, { "op": "test", "path": "\/c%d", "value": 2 }, { "op": "test", "path": "\/e^f", "value": 3 }, { "op": "test", "path": "\/g|h", "value": 4 }, { "op": "test", "path": "\/i\\j", "value": 5 }, { "op": "test", "path": "\/k\"l", "value": 6 }, { "op": "test", "path": "\/ ", "value": 7 }, { "op": "test", "path": "\/m~0n", "value": 8 } ]' : OK
 Testing 'Move to same location has no effect', doc '{ "foo": 1 }' patch '[ { "op": "move", "from": "\/foo", "path": "\/foo" } ]' : OK
@@ -129,7 +142,7 @@ Testing 'missing from parameter to move', doc '{ "foo": 1 }' patch '[ { "op": "m
  => json_patch_apply failed as expected: ERRNO=EINVAL at patch idx 0: Patch does not contain a 'from' field
 Testing 'missing from location to move', doc '{ "foo": 1 }' patch '[ { "op": "move", "from": "\/bar", "path": "\/foo" } ]' : OK
  => json_patch_apply failed as expected: ERRNO=ENOENT at patch idx 0: Did not find element referenced by from field
-Testing 'duplicate ops', doc '{ "foo": "bar" }' patch '[ { "op": "move", "path": "\/baz", "value": "qux", "from": "\/foo" } ]' : SKIPPING - disabled in the test spec
+Testing 'duplicate ops, json-c parses this as op:move', doc '{ "foo": "bar" }' patch '[ { "op": "move", "path": "\/baz", "value": "qux", "from": "\/foo" } ]' : OK
 Testing 'unrecognized op should fail', doc '{ "foo": 1 }' patch '[ { "op": "spam", "path": "\/foo", "value": 1 } ]' : OK
  => json_patch_apply failed as expected: ERRNO=EINVAL at patch idx 0: Patch object has invalid 'op' field
 Testing 'test with bad array number that has leading zeros', doc '[ "foo", "bar" ]' patch '[ { "op": "test", "path": "\/00", "value": "foo" } ]' : OK