JSON-Patch icon indicating copy to clipboard operation
JSON-Patch copied to clipboard

Custom Comparison Function

Open gaurav21r opened this issue 8 years ago • 5 comments

As in the README, I find this project to be way ahead of the competition, in terms of not only performance but also "community activity".

One thing that's missing though is a Custom Comparison Function. Eg: https://github.com/cujojs/jiff#diff https://github.com/benjamine/jsondiffpatch/blob/master/docs/arrays.md#an-object-hash

Can we implement a custom objectComparison function to compare() ? As outlined in the above link, incase of Array items changing positions, LCS fails!

gaurav21r avatar Jan 09 '16 07:01 gaurav21r

@warpech @Starcounter-Jack @miyconst @tomalec Your views please? I think this is pretty important. Most developers would use this library to diff their model. And they would usually have an id attribute. Maybe I'm just missing the custom comparator API from the docs?

gaurav21r avatar Jan 13 '16 06:01 gaurav21r

@gaurav21r sorry for the late response.

Am I right that you are trying to get "move" patch for array comparison instead of "replace"?

If so, then those examples would demonstrate your issue:

  • https://jsfiddle.net/rfLhkfs3/1/
  • https://jsfiddle.net/rfLhkfs3/2/

Moving an item in an array generates change patch for every index, instead of one move patch.

If so, then I am afraid that JSON-Patch currently does not support LCS algorithm to compare arrays.

@tomalec, @Starcounter-Jack am I right?

miyconst avatar Jan 13 '16 06:01 miyconst

@miyconst No Problem! :) Yes this is exactly the case. Can you look at the links I posted? These libraries use a customFunction argument to compare elements within an array. I think this is a very popular use case, because most of the data models are modeled as array of objects.

I am assuming it would a bit easy to implement. Because instead of using LCS or some other complex algorithm, we just have to check for the presence of a callback function and pass on the objects to compare to this function.

I'll try my hand at implementing it. But I need some help navigating the codebase as well as thoughts from the core team. The above 2 links can articulate the solution better than me.

gaurav21r avatar Jan 13 '16 07:01 gaurav21r

Well, the custom comparison function would not solve your issue, as JSON-Patch currently does not support move operation. As I have demonstrated in my first example with strings, which do not require custom comparison function, you still get array of change patches instead of one move patch.

I do understand your problem, and what you are trying to do, but unfortunately it is not possible right now. @tomalec should have more input on how this could be achieved and when it could be implemented.

miyconst avatar Jan 13 '16 07:01 miyconst

JSON-Patch currently does not support move operation

I'd say our fastjsonpatch.compare does not generate move operations, as .apply supports it ;)

Lack of move support in compare was already reported at https://github.com/Starcounter-Jack/JSON-Patch/issues/91 and we deffinatelly need to support it at some point.

Regarding additional parameter for better interoperability/hacking/plugins, we can consider it, but we need to make sure it will not affect performance, as we try to weight every if.

As it seems to be rather non trivial work, I'll not have much time to do it within 2-3 months, so PRs are as always welcome :)

tomalec avatar Jan 13 '16 15:01 tomalec