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.78k stars 215 forks source link

Array patching, over empty elements.. #287

Open KpjComp opened 2 years ago

KpjComp commented 2 years ago

has a slight problem with empty array elements and applying diffs.

Lets assume we have we have an array [1,,3,,5], and then create a patch for [1,,3,,5,,7,,9], The patch created is correct, eg: add for array element 6 & 8 with values 7 & 9. But if we apply this patch to the original, you will get [1,,3,,5,7,9].. IOW: it will scrunch the newly added array elements and put the first array element at index 5 & 6, instead of 6 & 8.

This function ->

var arrOps = {
    add: function (arr, i, document) {
        if (helpers_js_1.isInteger(i)) {
            arr.splice(i, 0, this.value);
        }
        else { // array props
            arr[i] = this.value;
        }
        // this may be needed when using '-' in an array
        return { newDocument: document, index: i };
    },

Now I'm not 100% sure why splice was used here, unless it's maybe a performance optimization, but the problem with splice here, if you did splice(6, 0, value), the index position it will use if the index is greater than length, is the length (wrong). IOW: It will update index 5, and not index 6, and then index 8 will be 6 (again because of length).

Now a simple solution is just don't use splice, and use arr[i] = , or alternatively set the length of the array first before doing splice.

wickning1 commented 2 years ago

The spec says "The specified index MUST NOT be greater than the number of elements in the array."

I think the error here is on the creation of the patch. It should be adding undefined elements before the 7 and the 9.

KpjComp commented 2 years ago

@wickning1 After doing the modification I mention, I've not had any issues with patching arrays.

But what is odd is that this issue is still marked OPEN, and for such a simple bug, with a very simple fix, this worries me a tad. IOW: is this project dead?