Fixing JsonPatchDocument bug

[HPOD00019]

Fixing JsonPatchDocument bug with Replace json array item

There was a bug when try using

JsonPatchDocument.Replace("/SomeArray/0/SomeField/", "new value"); 

during apply attempt the exception was thrown:

Microsoft.AspNetCore.JsonPatch.SystemTextJson.Exceptions.JsonPatchException: "For operation 'replace', the target location specified by path '/items/0/id/' was not found."

Description

Problem was in ListAdapter file, method TryTraverse consumed target parameter as "object" type and then tried to cast it to IList, but JsonArray implements only generic IList (not non-generic IList), so I added condition to proccess it any IList implementations correctly (instead of just returning null).

Also added one test to demonstrate problem (it runs with my insertations).

Fixes #65409

[HPOD00019]

@dotnet-policy-service agree

[adityamandaleeka]

Thanks for the fix @HPOD00019. I'd suggest a simpler approach though. Every other method in ListAdapter handles this dual IList/JsonArray support through GenericListOrJsonArrayUtilities. TryTraverse was the only method that bypassed it, which I think was an oversight. Rather than adding a second code path for IList<JsonNode>, we can just use the same utility here, which should fix this issue.

[adityamandaleeka]

Please let us know if you'd like to update the PR yourself or if you'd prefer we can take care of it.

[HPOD00019]

Thanks for your answer!
Did not notice that at first, I'll fix it right away!

[HPOD00019]

After refactoring, I noticed that test

Microsoft.AspNetCore.JsonPatch.SystemTextJson.Internal.ObjectVisitorTest.Visit_InvalidIndexFormatToArray_Fails

is now failing.
The test intentionally tries to access the last item in an empty array using '-' (which becomes -1 after parsing). It expects to get the error message:

Assert.Equal($"The path segment '{position}' is invalid for an array index.", message);

But now it throws an exception because TryTraverse attempts to access index -1.
I believe throwing an exception is actually the correct behavior here, since all other ListAdapter methods (except TryAdd) throw for invalid indices like -1 with the same input parameters.
Should I fix this test?

[adityamandaleeka]

The test shouldn't need to change. The issue is that TryTraverse shouldn't be using TryGetPositionInfo. It only needs to look up an existing element by numeric index so - isn't a valid input for it.

If you keep the original int.TryParse + bounds check logic, the existing test will continue to pass.

Would be good to have more tests here too btw... unit tests on ListAdapter.TryTraverse for both IList and JsonArray (including invalid index and out-of-bounds cases), and integration tests for replace, remove, add, and deeply-nested array traversal against JsonObject.

But feel free to leave the added testing for now and we can have Copilot add those easily.

[HPOD00019]

Fixed! Returned to int.TryParse and all tests are now passing.

I'll go ahead and add the additional tests you suggested

[HPOD00019]

I added some unit tests to ListAdatapterTest, tried to cover

• index out of bounds;
• valid index;
• invalid segment format;
• non generic forman (fails);
• arrat (fails).

I thought that duplicating all tests with IList and JsonArray would be too much so I added some specific tests with JsonArray in name.

[campersau]

This could be moved down after this if check and maybe even inlined into the if where it is actually checked, so the code is only executed when necessary.

[HPOD00019]

I agree.