Remove operation builds an incorrect path with duplicate indexes for arrays

[algorithm108]

Steps to Reproduce the Problem

Build diff using JsonDiff.asJson() with the following jsons:

1. Current

{
  "id": "Contract 1",
  "documents": [
    {
      "id": "1",
      "name": "First document"
    },
    {
      "id": "2",
      "name": "Second document"
    },
    {
      "id": "3",
      "name": "Third document"
    }
  ]
}

2. Modified

{
  "id": "Contract 1",
  "documents": [
    {
      "id": "1",
      "name": "First document"
    }
  ]
}

Removed last two objects from array

Expected Behavior

Json Patch document

[
    {
        "op": "remove",
        "path": "/documents/2"
    },
    {
        "op": "remove",
        "path": "/documents/1"
    }
]

Actual Behavior

Json Patch document

[
    {
        "op": "remove",
        "path": "/documents/1"
    },
    {
        "op": "remove",
        "path": "/documents/1"
    }
]

Specifications

http://jsonpatch.com/
for jsonpatch used recommended https://json-patch-builder-online.github.io/

Library Version:
0.4.11

Notes:
private void removeRemaining(JsonPointer path, int pos, int srcIdx, int srcSize, JsonNode source) {
while (srcIdx < srcSize) {
JsonPointer currPath = path.append(pos);
if (flags.contains(DiffFlags.EMIT_TEST_OPERATIONS))
diffs.add(new Diff(Operation.TEST, currPath, source.get(srcIdx)));
diffs.add(Diff.generateDiff(Operation.REMOVE, currPath, source.get(srcIdx)));
srcIdx++;
}
}

this function build diffs from starting pos, but not from exact index.

[holograph]

That is actually correct behavior; patches are applied on the state of the target object that they operate on. This means the two options are actually equivalent ([1,2,3] -> [1,2] -> [1] vs [1,2,3] -> [1,3] -> [1]).

The implementation is free to choose whichever option it prefers; is there any advantage to the proposed alternative?

[SquirrelGrip]

While both ways work, there is a single advantage to having the remove start from the last element and work towards the first element. The advantage comes when performing operation transformation.

Given a document {a:[1,2,3]}
Create patch A for {a:[1,3]} => [{"op": "remove","path": "/a/1"}]
Create patch B for [a:[]} => [{"op": "remove","path": "/a/0"},{"op": "remove","path": "/a/0"},{"op": "remove","path": "/a/0}]

When applying both patches to the document, I would like to transform the second patch as it will be doing repeated work. However, because I removed the second element, there is no operation in path B that corresponds to the second element being removed. Hence all path B operations are returned as changes, however applying the patch will fail as there are too many operations for the elements in the array and an error occurs.

However, if patch B was given as...
[{"op": "remove","path": "/a/2"},{"op": "remove","path": "/a/1"},{"op": "remove","path": "/a/0}]

The transformation applied to patch B would remove the 2nd operation is has already been removed. Once it is removed, the first operation will need to be adjusted to point to /a/1 as the patch A operation array index is greater. As the array gets longer, this makes more sense.

Also if the patches are applied in reverse order (B then A), it can be seen that the operation in A would have already been applied and could be removed, leaving patch A with no operations to apply.

Because of the order of removal being important it make transformation easier. But aside from that it makes no difference.

Would be good to know that we could may applying a setting generate patches in a different order when required.