search

Starcounter-Jack / JSON-Patch

Lean and mean Javascript implementation of the JSON-Patch standard (RFC 6902). Update JSON documents using delta patches.
MIT License
1.81k stars 215 forks source link

Replacing root object with an array doesn't work #147

Closed nicksnyder closed 7 years ago

nicksnyder commented 7 years ago

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 commented 7 years ago

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

alshakero commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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.

nicksnyder commented 7 years ago

@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/

screen shot 2017-02-20 at 8 34 48 pm screen shot 2017-02-20 at 8 34 19 pm

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

alshakero commented 7 years ago

@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.

image

warpech commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

FYI, my PR to add tests for this case was accepted https://github.com/json-patch/json-patch-tests/pull/30

felixfbecker commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

@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

felixfbecker commented 7 years ago

well, it would be possible to create a Proxy for an object that then holds a reference to the root object. That way you could update the root as well. But I personally do not have a use case for generating patches automatically, I just need to apply them, including setting the root value initially.

felixfbecker commented 7 years ago

@alshakero Are you planning to fix this? I took a shot at it but really don't understand the codebase well enough to understand what's going on. I currently need to use the much slower library by bruth which handles this correctly.

alshakero commented 7 years ago

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

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

This one is breaking.

felixfbecker commented 7 years ago

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.

alshakero commented 7 years ago

@felixfbecker excellent.

alshakero commented 7 years ago

@felixfbecker would you mind reviewing the PR https://github.com/Starcounter-Jack/JSON-Patch/pull/167?