when adding more than one value to a list, applying cheapDiff patch produces out of bound error

[eniac314]

I may have found another bug:

module CheapDiffTests exposing (..)

import Json.Decode as D
import Json.Diff as Diff exposing (..)
import Json.Encode as E
import Json.Patch as JP exposing (..)
import Json.Patch.Invertible as Inv exposing (..)

doc3 =
    E.list E.int [ 1, 2, 3, 4 ]


doc4 =
    E.list E.int [ 1, 2, 3, 4, 5, 6 ]


patch2 =
    Diff.diff doc3 doc4


cheapPatch2 =
    Diff.cheapDiff doc3 doc4 |> Inv.toMinimalPatch


val3 =
    JP.apply patch2 doc3 |> Result.toMaybe |> Maybe.map (D.decodeValue <| D.list D.int)


val4 =
    JP.apply cheapPatch2 doc3 |> Result.toMaybe |> Maybe.map (D.decodeValue <| D.list D.int)

val3 produces the correct value, val4 (when keeping the Result) gives this error: Err ("index out of bounds: 5")

[Lattyware]

I can reproduce this with the test case:

            {
                "description": "add values to list",
                "a": [1, 2, 3, 4],
                "b": [1, 2, 3, 4, 5, 6],
                "patch": [{"op": "add", "path": "/4", "value": 5}, {"op": "add", "path": "/5", "value": 6}]
            }

The issue seems to be it's creating the patch [{"op":"add","path":"/5","value":6},{"op":"add","path":"/4","value":5}], so it blows up trying to add the item beyond the end of the list.

We intentionally do the operations on lists in reverse, presumably because that makes sense for removals to avoid the inverse problem, but that's obviously a flawed approach.

We should probably do a check on the size of the two lists, and if the new one is longer, go forward, if it's shorter, go backwards, as with the simple approach the cheap diff uses we only get one or the other.

[Lattyware]

OK, #6 has a fix for this in, please take a look and tell me if it looks good for you, if so I'll put a new version out.

[eniac314]

Yes it works now, thank you!