Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Patch arrays generated after removing elements differs #16

Closed
psmolenski opened this issue Aug 13, 2013 · 5 comments
Closed

Patch arrays generated after removing elements differs #16

psmolenski opened this issue Aug 13, 2013 · 5 comments
Labels
Milestone

Comments

@psmolenski
Copy link
Contributor

If we remove items from an array using Array.prototype.splice() the order of patches generated by jsonpatch.generate() differs, depending on whether jsonpatch.observe() uses native Object.observe or a shim.

Lets take a sample test case:

it('should generate the same patch using Object.observe and shim after removing items from array', 
function() {
      var arr1 = [
        ["Albert", "Einstein"],
        ["Erwin", "Shrodinger"]
      ];

      var arr2 = arr1.slice();

      var observer1 = jsonpatch.observe(arr1);
      arr1.splice(0, 2);

      var objectObservePatches = jsonpatch.generate(observer1);

      var _observe = Object.observe;
      Object.observe = undefined;

      var observer2 = jsonpatch.observe(arr2);
      arr2.splice(0, 2);

      var shimPatches = jsonpatch.generate(observer2);

      expect(objectObservePatches).toEqual(shimPatches); //fails

      Object.observe = _observe;
});

the value stored in objectObservePatches is

[ { op : 'remove', path : '/1' }, { op : 'remove', path : '/0' }, 
{ op : 'replace', path : '/length', value : 0 } ]

whereas shimPatches contains array:

[ { op : 'remove', path : '/0' }, { op : 'remove', path : '/1' } ]

Ignoring patches related to length property (#14) patches generated by Object.observe() version indicate that we first removed item with index 1 and then 0 whereas shim version suggests something opposite.

@Starcounter-Jack
Copy link
Owner

Thanks for reporting this. When it comes to the order for the patches in the shim, they should be reversed (as you pointed out).

When it comes to the issue of the length property, I'm not sure which way to go. length is not an enumerable property, so should we generate a patch for it? As it is ignored by JSON.stringify(), the default behaviour should perhaps be that it is ignored by the JSON-Patch generator.

WDYT?

@warpech
Copy link
Collaborator

warpech commented Oct 10, 2013

The length property has it's own ticket: #14. I think it should be removed from the output

@warpech
Copy link
Collaborator

warpech commented Oct 11, 2013

Native Object.observe is reporting items removed with Array.prototype.splice in reverse order. In contrary, the shimmed version reports removed items in the natural order.

As we see here, different implementations may produce different results depending on the direction of the algorithm. When more browsers support Object.observe, that may get even more diverse if you compare the results as strings.

To me, the only logical way to sort patches is by the time of detection. Because we have no control over order of changes made internally in Array.prototype.splice, we need to agree that this may be reverse order if the browser implements it so.

Native Object.observe may know the real order of changes, but shim may only traverse the tree on time intervals, and then report the changes in natural (alphabetical) order.

Thus, in my opinion this issue should be Won't fix, but I leave this open for discussion.

@warpech
Copy link
Collaborator

warpech commented Oct 16, 2013

Now I have a new light on this issue... Considering this array:

var obj = {
  items: ["a", "b"]
}

Calling this will work OK:

jsonpatch.apply(obj, [
  {
    "op": "remove",
    "path": "/items/1"
  },
  {
    "op": "remove",
    "path": "/items/0"
  }  
])

But calling this will call exception:

jsonpatch.apply(obj, [
  {
    "op": "remove",
    "path": "/items/0"
  },
  {
    "op": "remove",
    "path": "/items/1"
  }
])

Because after first patch is applied, index 1 no longer exists in the items array. I think we need to change the shim implementation to iterate backwards so the array changes are always reported in descending index order.

@warpech
Copy link
Collaborator

warpech commented Oct 16, 2013

Fixed in 0.3.4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants