JSON-Patch: Replacing root object with an array doesn't work

I ran into two cases that this library does not seem to handle.

Apply diff to add these tests:

$ git diff
diff --git a/test/spec/json-patch-tests/tests.json b/test/spec/json-patch-tests/tests.json
index ec1c0c9..72a605d 100755
--- a/test/spec/json-patch-tests/tests.json
+++ b/test/spec/json-patch-tests/tests.json
@@ -55,6 +55,21 @@
       "expected": "bar",
       "disabled": true },
 
+    { "comment": "replace object document with array document?",
+      "doc": {},
+      "patch": [{"op": "add", "path": "", "value": []}],
+      "expected": [] },
+
+    { "comment": "replace array document with object document?",
+      "doc": [],
+      "patch": [{"op": "add", "path": "", "value": {}}],
+      "expected": {} },
+
     { "comment": "Add, / target",
       "doc": {},
       "patch": [ {"op": "add", "path": "/", "value":1 } ],

Test output:

Failures:
1) json-patch-tests tests.json should succeed: replace object document with array document?
  Message:
    Expected Object({  }) to equal [  ].
  Stack:
    Error: Expected Object({  }) to equal [  ].
        at Object.<anonymous> (/Users/nick/code/JSON-Patch/test/spec/jsonPatchTestsSpec.js:47:34)

2) json-patch-tests tests.json should succeed: replace array document with object document?
  Message:
    Expected [  ] to equal Object({  }).
  Stack:
    Error: Expected [  ] to equal Object({  }).
        at Object.<anonymous> (/Users/nick/code/JSON-Patch/test/spec/jsonPatchTestsSpec.js:47:34)

159 specs, 2 failures
Finished in 0.102 seconds



json-patch-tests tests.json should succeed: replace object document with array document?
  Expected Object({  }) to equal [  ].  Error: Expected Object({  }) to equal [  ].
      at stack (/Users/nick/code/JSON-Patch/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:1640:17)
      at buildExpectationResult (/Users/nick/code/JSON-Patch/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:1610:14)
      at Spec.expectationResultFactory (/Users/nick/code/JSON-Patch/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:655:18)
      at Spec.addExpectationResult (/Users/nick/code/JSON-Patch/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:342:34)
      at Expectation.addExpectationResult (/Users/nick/code/JSON-Patch/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:599:21)
      at Expectation.toEqual (/Users/nick/code/JSON-Patch/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:1564:12)
      at Object.<anonymous> (/Users/nick/code/JSON-Patch/test/spec/jsonPatchTestsSpec.js:47:34)
      at attemptSync (/Users/nick/code/JSON-Patch/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:1950:24)
      at QueueRunner.run (/Users/nick/code/JSON-Patch/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:1938:9)
      at QueueRunner.execute (/Users/nick/code/JSON-Patch/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:1923:10)

json-patch-tests tests.json should succeed: replace array document with object document?
  Expected [  ] to equal Object({  }).  Error: Expected [  ] to equal Object({  }).
      at stack (/Users/nick/code/JSON-Patch/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:1640:17)
      at buildExpectationResult (/Users/nick/code/JSON-Patch/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:1610:14)
      at Spec.expectationResultFactory (/Users/nick/code/JSON-Patch/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:655:18)
      at Spec.addExpectationResult (/Users/nick/code/JSON-Patch/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:342:34)
      at Expectation.addExpectationResult (/Users/nick/code/JSON-Patch/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:599:21)
      at Expectation.toEqual (/Users/nick/code/JSON-Patch/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:1564:12)
      at Object.<anonymous> (/Users/nick/code/JSON-Patch/test/spec/jsonPatchTestsSpec.js:47:34)
      at attemptSync (/Users/nick/code/JSON-Patch/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:1950:24)
      at QueueRunner.run (/Users/nick/code/JSON-Patch/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:1938:9)
      at QueueRunner.execute (/Users/nick/code/JSON-Patch/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:1923:10)

159 specs, 2 failures

About this issue

Original URL
State: closed
Created 7 years ago
Reactions: 2
Comments: 20 (5 by maintainers)

Commits related to this issue

Implement applyPatch to fix #147 — committed to Starcounter-Jack/JSON-Patch by alshakero 7 years ago

Most upvoted comments

That behaviour is not defined in the spec. The root path is mentioned specifically in the add spec:

When the operation is applied, the target location MUST reference one of:

The root of the target document - whereupon the specified value becomes the entire content of the target document.

but not for other ops. Given replace can be viewed as a remove + add, and add is valid for the root path, it may make sense to not error but set the root to null.

felixfbecker on May 9, 2017

@felixfbecker I’ll go for it ASAP. What should happen if someone applied

{op: "remove", path: ""}

This one is breaking.

alshakero on May 9, 2017

Imo this is a major shortcoming. It’s true that with the current API it is not possible to update the root to a new object reference (without using Object.setPrototypeOf() of course, which is super slow). But letting apply() always return the (possibly new) root value would be a very easy and backwards-compatible way to enable this use case for those who need it.

felixfbecker on Apr 30, 2017

That’s a good point @alshakero, I missed that.

Does fast-json-patch already correctly support replacing the root of an object to an object and root of an array to an array?

As the name of our library in NPM suggests, we aim to be fast and robust. We should not make any changes that degrade the performance.

We want to make it possible to use fast-json-patch with external proxies and replacing the root object with a new instance will not work there. Maybe we should just throw an error saying that this is not supported?

warpech on Feb 21, 2017

If this PR is accepted in https://github.com/json-patch/json-patch-tests/pull/30, then I agree we should implement it, especially given that we actually support JSON array as the JSON root, as presented in the above comment.

warpech on Feb 17, 2017

That’s true, but a JS array doesn’t qualify as a JSON Object. It does qualify as a JS object, but not a JSON object.

I don’t understand why you think this distinction is relevant.

JSON Patch defines a JSON document structure for expressing a sequence of operations to apply to a JavaScript Object Notation (JSON) document;

I don’t think it matters that a JSON array is not a JSON object; a JSON array is still a valid JSON document.

This is a valid JSON document:

["hi"]

This fact is re-iterated in section 3 of RFC 6902

A JSON Patch document is a JSON [RFC4627] document that represents an array of objects. [ { “op”: “test”, “path”: “/a/b/c”, “value”: “foo” }, { “op”: “remove”, “path”: “/a/b/c” }, { “op”: “add”, “path”: “/a/b/c”, “value”: [ “foo”, “bar” ] }, { “op”: “replace”, “path”: “/a/b/c”, “value”: 42 }, { “op”: “move”, “from”: “/a/b/c”, “path”: “/a/b/d” }, { “op”: “copy”, “from”: “/a/b/d”, “path”: “/a/b/e” } ]

Official tests also confirm that it is ok to have an array as a top level document (afaik this lib passes this test).

{ "comment": "toplevel array, no change",
       "doc": ["foo"],
       "patch": [],
       "expected": ["foo"] },

So why is it unreasonable to do an add on the root document to change its type?

nicksnyder on Feb 15, 2017

I submitted a PR to add these tests to the official tests here: https://github.com/json-patch/json-patch-tests/pull/30

nicksnyder on Feb 15, 2017