-
Notifications
You must be signed in to change notification settings - Fork 85
Description
We have been attempting to upgrade to 2.5 from 2.3 but have run into a few issues.
One of the fixes that worked for us was resolved by this pr, however it also has implemented another regression in the library. (potentially intentional)
From the PR
The last release introduced a bug that incorrectly handled object changes within arrays: #73. The objecthash functionality, which allows user-defined comparison for objects in an array, was initially implemented with a default to the hash of the object itself. The correct behavior, if this library is to match the behavior of the original library, is to default to index matching and not full object comparison in array diffs. This PR corrects the default behavior to do just that.
This change has created a regression of the default handling of arrays (without object hash function defined) the way that 2.3 handled them.
I have created some unit tests on two branches and can show the difference between 2.3 and 2.5 that previously worked and now don't.
2.3 (PR #72)
I can see that we want to align to the default handling so keen for your advice here:
- Should we update our code to implement our own Deep comparison?
- Should we put in a fix on the library to restore the 2.3 behavior when a object hash is not specified?
More details pinpointed change below
The problem:
Looking at the current MatchArrayElement method in [ItemMatch.cs:29-42](https://github.com/wbish/jsondiffpatch.net/blob/9ecc72c9a45c520582bc7f25073d6b496e5ddf55/Src/JsonDiffPatchDotNet/ItemMatch.cs#L29), I can see the issue:
public virtual bool MatchArrayElement(JToken object1, int index1, JToken object2, int index2)
{
if(ObjectHash != null)
{
return Match(object1, object2, ObjectHash);
}
if(object1.Type != JTokenType.Object && object1.Type != JTokenType.Array)
{
return JToken.DeepEquals(object1, object2);
}
return index1 == index2;
}
The issue is on the final return index1 == index2;. When there's no ObjectHash specified and the tokens are objects/arrays, it falls back to comparing indices instead of doing deep equality comparison.
This means that when array elements shift positions (like inserting an element at the beginning), objects that are identical in content are considered different because their indices don't match.
- Working behavior (old version): The ArrayDiff method used
itemMatch.Match(left[index], right[index])which would fall back to
JToken.DeepEquals()for content comparison when no ObjectHash was provided. - Broken behavior (current version): The ArrayDiff method now uses
itemMatch.MatchArrayElement(left[index], index1, right[index], index2),
and the MatchArrayElement method incorrectlyreturns index1 == index2for objects/arrays when no ObjectHash is provided.
This means:
- When an object is inserted at the beginning of an array, all subsequent objects shift indices
- The algorithm thinks all shifted objects are "different" because their indices don't match
- Instead of detecting a single insertion, it treats everything as changes/replacements