JSON Patch - remove operation by index appears to throw JsonPatchException (... is out of bounds of the array size) despite not appearing to be out of bounds

Hi all,

Describe the bug

I cannot be sure when this first started, but it may have been after migrating to .net5 - when doing multiple JSONPatch remove operations within applyTo() - an exception is thrown;

Microsoft.AspNetCore.JsonPatch.Exceptions.JsonPatchException: The index value provided by path segment '2' is out of bounds of the array size.

(in this case segment 2 - but this may refer to any index).

From looking at the underlying domain object, and the patch document - I can see no violation of the array index and I would expect the PATCH to succeed.

Exceptions (if any)

Microsoft.AspNetCore.JsonPatch.Exceptions.JsonPatchException: The index value provided by path segment '2' is out of bounds of the array size. at Microsoft.AspNetCore.JsonPatch.Internal.ErrorReporter.<>c.<.cctor>b__1_0(JsonPatchError error) at Microsoft.AspNetCore.JsonPatch.Adapters.ObjectAdapter.Remove(String path, Object objectToApplyTo, Operation operationToReport) at Microsoft.AspNetCore.JsonPatch.Adapters.ObjectAdapter.Remove(Operation operation, Object objectToApplyTo) at Microsoft.AspNetCore.JsonPatch.Operations.Operation1.Apply(TModel objectToApplyTo, IObjectAdapter adapter) at Microsoft.AspNetCore.JsonPatch.JsonPatchDocument1.ApplyTo(TModel objectToApplyTo, IObjectAdapter adapter) at Microsoft.AspNetCore.JsonPatch.JsonPatchDocument`1.ApplyTo(TModel objectToApplyTo)

Further technical details

.NET SDK (reflecting any global.json): Version: 5.0.400 Commit: d61950f9bf

Runtime Environment: OS Name: Windows OS Version: 10.0.19043 OS Platform: Windows RID: win10-x64 Base Path: C:\Program Files\dotnet\sdk\5.0.400\

Host (useful for support): Version: 5.0.9 Commit: 208e377a53

.NET SDKs installed: 5.0.302 [C:\Program Files\dotnet\sdk] 5.0.400 [C:\Program Files\dotnet\sdk]

.NET runtimes installed: Microsoft.AspNetCore.All 2.1.29 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All] Microsoft.AspNetCore.App 2.1.29 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.AspNetCore.App 3.1.18 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.AspNetCore.App 5.0.8 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.AspNetCore.App 5.0.9 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.NETCore.App 2.1.29 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.NETCore.App 3.1.18 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.NETCore.App 5.0.8 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.NETCore.App 5.0.9 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.WindowsDesktop.App 3.1.18 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App] Microsoft.WindowsDesktop.App 5.0.8 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App] Microsoft.WindowsDesktop.App 5.0.9 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]

Or am I totally failing to understand the nature of multiple removes from the same collection within a single applyTo()? Am I supposed to understand the shifting of the collection per actual remove as to "figure" out what the indexes should be upfront?

so it seems that I did indeed not understand how JSONPatch removes work - I guess there is either an assumption that you are only going to remove a single item from a collection during applyTo() or that you need to account for shifting indexes yourself. It's not massively complex to apply the relevant shifting up front (as long as the assumption is that JSONPatch will execute the operations in the order that they exist in the Operationscollection.

so this is my workaround - and I execute this prior to calling applyTo(). I can't even begin to think about how shifting would interplay if there were also add operations on the same collection...?

private static bool AdjustRemoveOperationsForShiftingDelete<T>(JsonPatchDocument<T> patchDocument)
            where T: BaseEntity
        {
            //if first remove operation and more present, don't shift but increase shift-count
            //if > 0 index, reduce index value by shift count
            var removeParticipantOperations =
                patchDocument.Operations.Where(o => o.OperationType == OperationType.Remove).ToList();

            //if just a single remove, or no removes - then there is no need to shift
            if (removeParticipantOperations.Count < 2)
                return true;

            int shiftCount = 0;
            for (var i = 0; i < removeParticipantOperations.Count; i++)
            {
                if (i == 0)
                {
                    shiftCount++;
                    continue;
                }

                try
                {
                    //get the index, shift it according to shift count and then update shift count
                    string path = removeParticipantOperations[i].path;
                    string[] pathParts = path.Split("/");

                    int preShiftIndex =
                        int.Parse(pathParts.Last());

                    int shiftedIndex = preShiftIndex - shiftCount;

                    removeParticipantOperations[i].path = string.Join('/', pathParts.Take(pathParts.Length - 1));
                    removeParticipantOperations[i].path += $"/{shiftedIndex}";
                    shiftCount++;

                }
                catch
                {
                    return false;
                }
            }

            return true;
        }

this workaround feels really hacky, effectively shifts the indexes down according to how many entries are already going to be removed earlier in the collection - for example, if you had a patchDocument with 5 remove operations against a collection, and you wanted to remove all 5 - you would actually shift these such that you have 5 remove operations all pointing to /collection/0 ... hardly intuitive

Actually.. and ultimately, JSONPatch remove operations would be far nicer if they could work with an identifier rather than an index.

This awkwardness is a result of how the specification was written:

Evaluation of a JSON Patch document begins against a target JSON document. Operations are applied sequentially in the order they appear in the array. Each operation in the sequence is applied to the target document; the resulting document becomes the target of the next operation. https://tools.ietf.org/id/draft-ietf-appsawg-json-patch-10.html#rfc.section.3

If removing an element from an array, any elements above the specified index are shifted one position to the left. https://tools.ietf.org/id/draft-ietf-appsawg-json-patch-10.html#rfc.section.4.2

As annoying as it is to shift indexes, or delete indexes in reverse order, that is how the spec is written and I strongly believe that tooling should respect that.

thanks @darrelmiller - as long as we can all agree that it is indeed awkward! :)

Within our project, we may just move away from JSONPatch all together at some point, in favor of PUT operations. Using JSONPatch has been awkward from the backend perspective but equally seems to be an area that our front end API consumers struggle with too.

Also - please ignore my workaround above, it does not consider remove operation paths - and therefore is largely useless in it's current form.. :)

dotnet / aspnetcore