Replacing root object with an array doesn't work

[nicksnyder]

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

[nicksnyder]

I submitted a PR to add these tests to the official tests here:
json-patch/json-patch-tests#30

[alshakero]

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.

An object is an unordered set of name/value pairs. An object begins with { (left brace) and ends with } (right brace). Each name is followed by : (colon) and the name/value pairs are separated by , (comma).

Reference: http://www.json.org/

And the RFC defines JSON Patch as

JSON Patch defines a JSON document structure for expressing a
sequence of operations to apply to a JavaScript Object Notation
(JSON) document; it is suitable for use with the HTTP PATCH method.
The "application/json-patch+json" media type is used to identify such
patch documents.

Reference: https://tools.ietf.org/html/rfc6902

[nicksnyder]

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?

[warpech]

If this PR is accepted in json-patch/json-patch-tests#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.

[nicksnyder]

@warpech @tomalec Is there a reason you would wait for that to get merged before fixing it here? Even if the spec doesn't require this behavior (I believe that it does, but even if it doesn't), this seems like a reasonable thing for this library to handle correctly.

JSON 8 handles this correctly
https://json8.github.io/patch/demos/apply/

I don't see an argument for doing anything other than handling this case correctly (unless it is just a matter of effort).

[alshakero]

@nicksnyder fast-json-patch modifies the object in place. And please correct me if I'm wrong, but changing the object itself to an array would mean loosing its binding.

If you look at JSON8, it returns a brand new pointer that you can bind to a new variable that is unrelated to the original object, and that of course makes it possible. But as far as I know you can't replace an object with anything in place without loosing the binding.

[warpech]

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?

[alshakero]

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

Yes and yes.

var myobj = { firstName: "Albert" };
var patches = [{ op: "replace", path: "", value: { newObj: "very new" } }];
jsonpatch.apply(myobj, patches);
console.log(myobj); // outputs { newObj: 'very new' }

var myobj = [1];
var patches = [{ op: "replace", path: "", value: [2] }];
jsonpatch.apply(myobj, patches); 
console.log(myobj); // outputs [2]

[nicksnyder]

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.

I am not sure what you mean, mind elaborating?

Maybe we should just throw an error saying that this is not supported?

This seems like a reasonable thing to do if supporting this would compromise other design goals.

You could also implement a version of apply that does return a value (so it can return a new reference if necessary). Most operations could still be done in place.

[felixfbecker]

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.

[nicksnyder]

FYI, my PR to add tests for this case was accepted
json-patch/json-patch-tests#30

[felixfbecker]

Having an apply() variant that just takes a document and a single patch and returns the updated document would make it perfect for reduce():

const result = patches.reduce(apply, null) // replace null with initial document value

Makes it super easy to work with an RxJS Stream of JSON patches, for example.

[alshakero]

@felixfbecker that sounds great! Thank you.

That's great because the last suggestion of making current apply return the possibly new object would break a lot of code in our codebases. We use the returned results array to notify DOM elements when a patch is applied. So a separate function sounds much better and easier to implement.

@tomalec WDYT about applyPatch?

[alshakero]

@nicksnyder as for your question on:

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.

We now have a brand new implementation of object observation using ES6 proxies. And this method returns a new Proxy object that we can't afford to replace. Any change to this object emits a patch synchronously, and this makes it much more semantic and less error-prone. This implementation is only for observation and doesn't have apply or validate in it. So it has to play with jsonpatch well. And this means object replacement isn't of much use to us.

You can check it here https://github.com/Palindrom/JSONPatcherProxy