Skip to content

Added System.Text.Json serialization-based JsonPatch implementation #61313

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

mkArtakMSFT
Copy link
Member

@mkArtakMSFT mkArtakMSFT commented Apr 4, 2025

Fixes #24333

Design document: https://gist.github.com/mkArtakMSFT/346cf9d5c6eeb73406185751e28bd8fa

Benchmarks

I've a few benchmarks to compare this implementation with the original implementation and below are results for a few scenarios that I wanted to highlight.

Type Method Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
ApplicationBenchmarks NewtonsoftJsonPatch 271.924 us 5.4014 us 12.8370 us 1.9531 1.4648 - 25 KB
ApplicationBenchmarks SystemTextJsonPatch 1.584 us 0.0307 us 0.0399 us 0.2365 - - 3 KB
DeserializationBenchmarks NewtonsoftJsonPatch 19.261 us 0.4241 us 1.2439 us 3.5095 0.0610 - 43 KB
DeserializationBenchmarks SystemTextJsonPatch 7.917 us 0.3401 us 0.9866 us 0.5493 - - 7 KB

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Apr 4, 2025
@mkArtakMSFT mkArtakMSFT marked this pull request as ready for review April 16, 2025 22:30
@mkArtakMSFT mkArtakMSFT force-pushed the mkArtakMSFT/issue_24333 branch from 455a279 to 7483c60 Compare April 17, 2025 16:15
@danmoseley danmoseley requested a review from Copilot April 17, 2025 23:19
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces a System.Text.Json-based JSON Patch implementation, adding new converters, adapters, and helper utilities to support JSON Patch operations using System.Text.Json.

  • Implements a new JSON serialization-based approach for JSON Patch.
  • Adds several converters for both generic and non-generic JSON Patch documents.
  • Introduces utility classes and adapters to manipulate JSON structures and apply patch operations.

Reviewed Changes

Copilot reviewed 68 out of 73 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/Features/JsonPatch.SystemTextJson/src/Internal/ConversionResult.cs New helper class for holding conversion results.
src/Features/JsonPatch.SystemTextJson/src/IJsonPatchDocument.cs Defines the JSON Patch document interface with operations.
src/Features/JsonPatch.SystemTextJson/src/Helpers/JsonUtilities.cs Provides a deep comparison utility for JSON objects.
src/Features/JsonPatch.SystemTextJson/src/Helpers/GenericListOrJsonArrayUtilities.cs Utility methods for working with IList and JsonArray types.
src/Features/JsonPatch.SystemTextJson/src/Exceptions/JsonPatchException.cs Custom exception class for JSON Patch errors.
src/Features/JsonPatch.SystemTextJson/src/Converters/OperationConverterOfT.cs Converter for handling JSON Patch operations for generic types.
src/Features/JsonPatch.SystemTextJson/src/Converters/OperationConverterFactory.cs Factory for creating operation converters.
src/Features/JsonPatch.SystemTextJson/src/Converters/JsonPatchDocumentConverterFactory.cs Converter factory for both generic and non-generic JSON Patch documents.
src/Features/JsonPatch.SystemTextJson/src/Converters/JsonPatchDocumentConverter.cs Converter for non-generic JsonPatchDocument.
src/Features/JsonPatch.SystemTextJson/src/Converters/JsonConverterForJsonPatchDocumentOfT.cs Converter for generic JsonPatchDocument<T>.
src/Features/JsonPatch.SystemTextJson/src/Adapters/ObjectAdapter.cs Implements core operations (add, remove, replace, move, copy, test) for patching objects.
src/Features/JsonPatch.SystemTextJson/src/Adapters/IObjectAdapterWithTest.cs Interface for adapters supporting "test" operations.
src/Features/JsonPatch.SystemTextJson/src/Adapters/IObjectAdapter.cs Base interface for JSON Patch operations on objects.
src/Features/JsonPatch.SystemTextJson/src/Adapters/IAdapterFactory.cs Interface for adapter factory implementations.
src/Features/JsonPatch.SystemTextJson/src/Adapters/AdapterFactory.cs Default factory for creating appropriate adapters based on target type.
Files not reviewed (5)
  • AspNetCore.slnx: Language not supported
  • src/Features/JsonPatch.SystemTextJson/.vsconfig: Language not supported
  • src/Features/JsonPatch.SystemTextJson/JsonPatch.SystemTextJson.slnf: Language not supported
  • src/Features/JsonPatch.SystemTextJson/build.cmd: Language not supported
  • src/Features/JsonPatch.SystemTextJson/build.sh: Language not supported
Comments suppressed due to low confidence (2)

src/Features/JsonPatch.SystemTextJson/src/Converters/OperationConverterOfT.cs:13

  • The override of CanConvert is redundant as it only returns the base implementation's result. Removing this override would simplify the code.
public override bool CanConvert(Type typeToConvert) { var result = base.CanConvert(typeToConvert); return result; }

src/Features/JsonPatch.SystemTextJson/src/Converters/JsonConverterForJsonPatchDocumentOfT.cs:15

  • This CanConvert override is unnecessary since it directly calls and returns the base implementation's result. Consider removing it to reduce code redundancy.
public override bool CanConvert(Type typeToConvert) { var result = base.CanConvert(typeToConvert); return result; }

- Turn the IAdapter and IAdapterFactory types internal
@mkArtakMSFT
Copy link
Member Author

Thank you so much, @PranavSenthilnathan . I've adopted your converter implementation and the performance got so much better - much better than the original results I had with my implementation:

Type Method Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
ApplicationBenchmarks NewtonsoftJsonPatch 386.534 us 7.6784 us 13.4482 us 1.9531 0.9766 - 25 KB
ApplicationBenchmarks SystemTextJsonPatch 2.418 us 0.0480 us 0.0913 us 0.2289 - - 3 KB
DeserializationBenchmarks NewtonsoftJsonPatch 27.165 us 0.5290 us 0.7917 us 3.5095 0.0610 - 43 KB
DeserializationBenchmarks SystemTextJsonPatch 11.368 us 0.2243 us 0.4102 us 0.5493 - - 7 KB

More specifically, the Deserialization with my original model was 2x slower than the Newtonsoft.Json-based model, and the update makes it more than 2x faster than Newtonsoft.Json one.

return true;
}

if (a == null || b == null)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible for one of the inputs to be a JsonElement representing null while the other is just CLR null?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure, theoretically - maybe. I'd prefer to wait for actual feedback on this one from customers before trying to expand the implementation to cover that scenario too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JsonPatchDocument should use System.Text.Json
3 participants