chbrown / rfc6902

Complete implementation of RFC6902 in TypeScript
https://chbrown.github.io/rfc6902/
322 stars 39 forks source link

Improper patch created in certain scenarios #33

Closed bozzaj closed 6 years ago

bozzaj commented 6 years ago

Consider the following:

var obj = { "0": 4 }
var obj2 = [4];

rfc6902.createPatch() generates an empty patch.

However, the following works:

var obj = { "0": 4 }
var obj2 = [5];

The patch created is:

[
  {
    "op": "replace",
    "path": "",
    "value": [
      5
    ]
  }
]

It seems that a change from an object with numeric properties to an array is only detected as a change when one of the values changes.

bozzaj commented 6 years ago

I'm not assuming that. That's exactly my point. An object with a property of '0' is not equal to an array of one element containing the same value. The library should generate a diff in the first case when instead considers them equal and generates an empty patch.

chbrown commented 6 years ago

Fixed in 76f742a, which excludes Arrays from candidacy when comparing whether two objects are equal. IIRC, this was the original intent, but I was testing for object-hood with Object(x) === x, which indeed is a popular/common test for object-hood — as long as you assume that Arrays are "Objects".

I could imagine (and I considered) an argument for "if the key-value pairs are the same, the containers are the same", but {"0": "Hello"} and ["Hello"] are substantially different as far as JSON is concerned, which, to me, indicates that they ought to be treated differently.

FYI, the test in 12dc683 nests the numerically-indexed object inside an object container so that I can check that the created patch actually works (via applyPatch), since an object cannot be mutated into an array when passed by value. I.e., there is no function f that can achieve something like:

function f(value) { ... }
let x = {}
f(x)
Array.isArray(x) // will ALWAYS return false for any implementation of f
// please correct me if I'm wrong — that would be incredible! maybe some prototype wizardry?

Which means that for your original example, after setting obj = {"0": 4}, there is no patch such that applyPatch(obj, patch) will result in an obj that is an Array.

bdsbuild2 commented 6 years ago

Understood. I'm not aware of any wizardry to mutate in place, so no corrections there!

My original issue started with a much more complicated patch where the Object to Array was several levels deep. After tracking down the issue, I simplified it down to the root. I don't anticipate needing to actually change a root object.

Seems like this is, in a way, related to #32. No pass by reference, so no real way to change the type.