return removed items from apply

[joozek78]

Right now, there's no way to know values of objects that have been removed by apply. This could be alleviated by returning an array of removed elements from apply:

var obj = {name:'jack', lanugages: ['c#','haskell','python'], hobby:'music'};
var result = jsonpatch.apply(obj, [
  {op:'remove', path:'/hobby'},
  {op:'replace', path:'/name', value:'black'},
  {op:'add', path:'/languages/-', value:'orcish'},
  {op:'remove', path:'/languages/0'}
]);
assert result == [
  'music',
  'jack',
  undefined,
  'c#'
];

This, however poses a problem - apply already returns a boolean. Its return value is the result of 'test' operation if it is the last in patch or true otherwise. We could:

1. Introduce a function to perform single (or multiple) test operation and return just removed elements from apply
2. Return an object comprising test result and removed elements from apply
3. Introduce new argument to apply - an array which would be filled with removed elements (so-called 'out argument')

[joozek78]

cc @tomalec @miyconst @warpech

[MarkHerhold]

I personally think we should go with # 2 and break the API (and release v1 as a result) and return an object like so:

{
  success: true,
  removed: [...]
}

That being said, I am not a fan of the proposal itself or maybe I don't understand the use-case.

[warpech]

@joozek78 maybe your problem would be circumvented by using the following hack to retrieve the values before calling apply?

var obj = {name:'jack', lanugages: ['c#','haskell','python'], hobby:'music'};
var getExistingValue = {op: "_get", path: "/lanugages/2"};
jsonpatch.apply(obj, [getExistingValue]);
expect(getExistingValue.value).toEqual("python");

The _get operation is not documented, but it works. We can document it.

[joozek78]

The problem is, I don't need single value, I could just access the object directly. I need to know the values of any objects that were removed during apply

[warpech]

You can have exactly that, just replace the op with _get in your sequence of patches:

var obj = {name:'jack', languages: ['c#','haskell','python'], hobby:'music'};
var getExistingValues = [
  {op:'_get', path:'/hobby'},
  {op:'_get', path:'/name', value:'black'},
  {op:'_get', path:'/languages/-', value:'orcish'},
  {op:'_get', path:'/languages/0'}
];
jsonpatch.apply(obj, getExistingValues);
var values = getExistingValues.map(function(val){
  return val.value;
});
expect(values).toEqual([ 'music', 'jack', undefined, 'c#' ]);

[tomalec]

According to RFC: if any test operation fails, entire sequence should be rolled back, and elements should be removed iff all tests passed. Therefore i propose to return:

• false any test operation failed
• an array if all tests passed
  • [undefined] if there was non-removing operation
  • [{objectThat:'was removed'}, ...] if there was operation that removed smth

Just to remind, we would like apply to return removed element(-s) in similar manner that Array.prototype.splice does - as in fact each individual operation object could potential by array-manipulating method. Thanks, to that we will be somehow consistent with other Array related methods, and will be able to do something with references to removed objects (for example use them for Polymer template binding API)

[tomalec]

to @warpech's

You can have exactly that, ..

The thing is that, to do so you need to know which operations will remove something, and I believe the apply is the one that posses this knowledge. And can return it almost for free.

[warpech]

When this issue is resolved, https://github.com/Starcounter-Jack/JSON-Patch/issues/64 becomes obsolete

[tomalec]

I think we have few things to polish after https://github.com/Starcounter-Jack/JSON-Patch/pull/110/files like I mentioned at https://github.com/Starcounter-Jack/JSON-Patch/commit/946ea2d5cedc76a5100603c308f31af144e22554

1. rootOps.add needs to return removed object https://github.com/Starcounter-Jack/JSON-Patch/blob/master/src/json-patch-duplex.ts#L157
2. add could use removed output, instead of re-calculating it again
3. I would not copy the object at https://github.com/Starcounter-Jack/JSON-Patch/blob/master/src/json-patch-duplex.ts#L158-L162 and https://github.com/Starcounter-Jack/JSON-Patch/blob/master/src/json-patch-duplex.ts#L166-L171, but rather return removed object.

[joozek78]

1. I changed add not to return removed objects at all. Adding onto existing objects is illegal per RFC (although we support it) and array add already returns an integer (index at which object has been inserted), so it would be confusing to have different return value on root.add
2. see above
3. AFAIK we can't lose the root object, because outside things can reference it. That's why all its properties are changed instead of simple assignment

[tomalec]

1. Regarding

   Adding onto existing objects is illegal per RFC

   As per RFC it is supported, see http://tools.ietf.org/html/rfc6902#section-4.1

   If the target location specifies an object member that does exist, that member's value is replaced.

   But I agree with returning different value than array add.

2. .

3. That's the thing, we are replacing the object with new one. So outside things are referencing the old one, not the new one. I suppose that when you are replacing, adding, removing a root element, you mean it - to really use new one, and forget about the old one. Otherwise you would do replace all the properties.

   As for

   var newObj = {n: 'smth'};
   var tree = {prop: {nested: 'value'}};

   • to replace prop with newObj you should do:

     {op: 'replace', path: '/prop', value: newObj`}

     then tree.prop === newObj

   • to replace prop.nested with smth you do

     {op: 'replace', path: 'prop/nested', value 'smth'`}

   Why it should behave differently for root obj?

   I propose that

   • to replace tree root with newObj you should do:

     {op: 'replace', path: '', value: newObj`}

     then tree === jsonpatch.apply(tree, [operationObj])[0], we stopped observing tree, and we are now observing newObj, and the newObj is the one you should now refer to.

   • to replace tree.prop with smth you do

     {op: 'replace', path: 'prop', value 'smth'`}

   • to remove tree.prop and add n: 'smth' you do

     [{op: 'remove', path: '/prop'}, {op: 'add', path: 'n', value 'smth'}]

However, now, I see that's probably a discussion for 2.0.0 or 1.1.0 at least.

[joozek78]

However now I see that's probably a discussion for 2.0 or 1.1 at least

Definitely, we should also consider how will it play with e.g. Polymer. I'm closing this one, will you create an issue for your proposed changes?

[tomalec]

Discussion moved to https://github.com/Starcounter-Jack/JSON-Patch/issues/113