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

Generated patch contains array `length` #14

Closed warpech closed 11 years ago

warpech commented 11 years ago

In native Object.observe version (Chrome 28 & 30), array length is reported as part of the change records.

I don't think this is an error in Chrome, but I think JSON-Patch should filter out length from it's own generated patch

See http://jsfiddle.net/U4mZJ/3/

For a sample array, generated patch is:

[
 {"op":"replace","path":"/0","value":99},
 {"op":"add","path":"/2","value":3},
 {"op":"replace","path":"/length","value":3}
]

I will fix this soon

psmolenski commented 11 years ago

I have prepared a sample test case:

it('should generate the same patch using Object.observe and shim', function() {
      var arr1 = [
        ["Albert", "Einstein"],
        ["Erwin", "Shrodinger"]
      ];
      var arr2 = arr1.slice();
      var newRecord = ['Niels', 'Bohr'];
      var observer1 = jsonpatch.observe(arr1);
      arr1.push(newRecord);
      var objectObservePatches = jsonpatch.generate(observer1);
      var _observe = Object.observe;
      Object.observe = undefined;
      var observer2 = jsonpatch.observe(arr2);
      arr2.push(newRecord);
      var shimPatches = jsonpatch.generate(observer2);
//fails because objectObservePathes has an extra patch with "length" property
      expect(objectObservePatches).toEqual(shimPatches); 
      Object.observe = _observe;
    });
warpech commented 11 years ago

After studying http://tools.ietf.org/html/rfc6902 and some sample test cases here https://github.com/mikemccabe/json-patch-tests I think the only good solution is to NOT include length in the produced patch. I will submit a fix

warpech commented 11 years ago

Thanks @psmolenski, I added your test to the test suite

warpech commented 11 years ago

This issue is fixed in v0.3.3

tomalec commented 9 years ago

It seems that such behavior is not a bug in Chrome, but desired behavior for Object.observe it's even mentioned in spec.

That's the main difference with Array.observe, which I believe we should use for array(-like) nodes.

tomalec commented 9 years ago

I'll clean our .length logic at https://github.com/Starcounter-Jack/JSON-Patch/commit/0552ac695bb007d3f488ad4c91c3dc7aa217bb47. So users may use 'length' property name for non-array objects. For arrays, we will handle splice change type.