Skip to content
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

JsonPatchDocument should use System.Text.Json in ASP.NET vNext #24333

Open
xperiandri opened this issue Jul 27, 2020 · 46 comments
Open

JsonPatchDocument should use System.Text.Json in ASP.NET vNext #24333

xperiandri opened this issue Jul 27, 2020 · 46 comments
Labels
affected-few This issue impacts only small number of customers area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-json-patch severity-minor This label is used by an internal tool
Milestone

Comments

@xperiandri
Copy link

Related to #16968
Will you accept pull-request with this feature?

@xperiandri
Copy link
Author

I propose to:

@mkArtakMSFT mkArtakMSFT added area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates feature-json-patch enhancement This issue represents an ask for new feature or an enhancement to an existing one labels Jul 27, 2020
@mkArtakMSFT mkArtakMSFT added this to the Backlog milestone Jul 27, 2020
@ghost
Copy link

ghost commented Jul 27, 2020

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

@mkArtakMSFT
Copy link
Member

Thanks for your willingness to help.
We would prefer to think more about this first and decide on the direction we want to go before asking for help.

@null-d3v
Copy link

There have been many issues reported regarding json patch still being tied to Newtonsoft.Json deserialization. The issues are caused by adverse effects of the association, or directly indicating dissatisfaction with it:
#12994
#13938
#14035
#16968
#22216
#22620

System.Text.Json has merits over Newtonsoft.Json purported by Microsoft with indication to fully remove the Json.NET dependency from ASP.NET Core.

Thus, we want to remove the Json.NET dependency from ASP.NET Core 3.0, so that customers can choose which version to use, without fearing they might accidentally break the underlying platform.

Configuration of json patch to only use Newtonsoft.Json processing for patch requests is contrived, requiring building an entire separate service collection with complete MVC to pull an input formatter with appropriate DI.

To indicate the intent of complete System.Text.Json support in ASP.NET Core, not have the support, and then to consistently delay in the face of consistent issues is extremely discouraging:
#12994 (comment)
#14035 (comment)
#14035 (comment)
#16968 (comment)

@erwan-joly
Copy link

I wonder if the easiest way to have that feature added wouldn't be to fork the feature project https://github.com/dotnet/aspnetcore/commits/master/src/Features/JsonPatch in another repo
make it works without newtonsoft
then ask dotnet/aspnetcore if they want to replace newtonsoft json patch to that one

@SteveSandersonMS SteveSandersonMS added affected-few This issue impacts only small number of customers severity-minor This label is used by an internal tool labels Oct 6, 2020 — with ASP.NET Core Issue Ranking
@ghost
Copy link

ghost commented Nov 11, 2020

This is bit of an annoying oversight. Any news?

@erwan-joly
Copy link

https://github.com/gregsdennis/json-everything

@natelaff
Copy link

natelaff commented Jan 14, 2021

+1, Everything is moving to System.Text.Json... Swagger, Refit, etc... this needs to be addressed.

@sulmar
Copy link

sulmar commented Jul 23, 2021

Do you have any plans to use JsonPatchDocument with System.Text.Json in upcoming .NET 6 ?
It will be very handy in Minimal API.

@Havunen
Copy link

Havunen commented Aug 23, 2021

Has anybody found good open source json patch library for system text json?

@erwan-joly
Copy link

Has anybody found good open source json patch library for system text json?

#24333 (comment)

@pranavkm pranavkm added area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels and removed area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates labels Oct 19, 2021
@winromulus
Copy link

A lot of projects have dropped support for Newtonsoft.Json (especially for annotations) in favor of System.Text.Json
Some projects, like the Kubernetes C# client rely heavily on support for JSON Patch.
Is it possible to re-evaluate the priority on this in light of increased requests for support on this topic? It's been years since it was considered low-priority since few projects had been ported to System.Text.Json at the time.

@Grizzlly
Copy link

@pranavkm I think this is worth looking into. It is a bummer that we have to rely on Newtonsoft.

The code in the documentation throws ASP0000 warning. This should be mentioned in the docs imo.

@beachwalker
Copy link

Still a issue? Keeps us from switching.

@Grizzlly
Copy link

Still a issue? Keeps us from switching.

Yes.

@rqueizan
Copy link

rqueizan commented Mar 7, 2022

Install this package Microsoft.AspNetCore.Mvc.Formatters.Json for System.Text.Json

@erwan-joly
Copy link

Install this package Microsoft.AspNetCore.Mvc.Formatters.Json for System.Text.Json

This seems to use newtonsoft from Microsoft.AspNetCore.JsonPatch

@rqueizan
Copy link

rqueizan commented Mar 7, 2022

Install this package Microsoft.AspNetCore.Mvc.Formatters.Json for System.Text.Json

This seems to use newtonsoft from Microsoft.AspNetCore.JsonPatch

image

image

image

I am not using Newtonsoft.Json!

@erwan-joly
Copy link

Install this package Microsoft.AspNetCore.Mvc.Formatters.Json for System.Text.Json

This seems to use newtonsoft from Microsoft.AspNetCore.JsonPatch

image

image

image

I am not using Newtonsoft.Json!

Indirectly your are https://www.nuget.org/packages/Microsoft.AspNetCore.JsonPatch/

@pdevito3
Copy link

So I was just playing around with what it might look like to convert my controllers to minimal APIs and noticed that we don't currently have MapPatch, but that it's coming in .NET 7.

@davidfowl @captainsafia I saw that you guys had some traffic on that in #36198 and want to raise the flag on this issue in case you're not aware. Thinking many people will reach for a Patchdoc when setting it up and this will block that option for them as far as I can tell -- unless there's a way to set that patch endpoint to use newtonsoft explicitly but even so, that would be somewhat hacky.

@davidfowl
Copy link
Member

@pdevito3 Thanks! I'm not sure we'll be able to pull this off for .NET 7 as patch support for System.Text.Json is currently unplanned but this is something we should get ahead of at least in the docs. Maybe we can write a sample converter to make this work in the mean time.

@Havunen
Copy link

Havunen commented May 21, 2022

I started implementing this myself, its based on aspnet version. https://github.com/Havunen/SystemTextJsonPatch

@davidfowl
Copy link
Member

@Havunen WONDERFUL! We should point people to this for now. Are there any other implementations that you are aware of?

@erwan-joly
Copy link

@Havunen WONDERFUL! We should point people to this for now. Are there any other implementations that you are aware of?

#24333 (comment)

@natilivni
Copy link

Any update on this?

@davidfowl
Copy link
Member

davidfowl commented Jun 9, 2022

We are not doing this for .NET 7, the work isn't planned and doesn't fit into the release based on the other work we're already doing.

PS: This item is in the backlog.

@stevendarby
Copy link

stevendarby commented Jun 16, 2022

Could MS please clarify what their commitment to AspNetCore.JsonPatch is, as it's looking pretty shaky to me. Reasons for thinking this:

  • STJ was released with .NET Core 3.0 in late 2019 and with no support now or in upcoming 7.0, late 2023 is the earliest it could be supported. 4 years!
  • Bugs and requests against it just seem to get backlogged (including two of mine) despite very little activity in that area. I think JsonPatch sharing a backlog with core ASP.NET Core means it'll always be deprioritised unless a conscious push is made.
  • Key MS developers are endorsing 3rd party libraries instead 😟

@Havunen
Copy link

Havunen commented Jun 17, 2022

Bugs and requests against it just seem to get backlogged (including two of mine) despite very little activity in that area. I think JsonPatch sharing a backlog with core ASP.NET Core means it'll always be deprioritised unless a conscious push is made

Can you try out SystemTextJsonPatch? I believe I have fixed both the issues you reported about AspNetCore.JsonPatch.
I know its not the official Microsoft package but personally I just needed a package that works and does not depend on Newtonsoft.

@hexjourney
Copy link

hexjourney commented Jan 17, 2023

This issue doesn't impact a small number of customers...

@carlblanchard
Copy link

Why isn't System.Text.Json adopting JsonPatchDocument?
It's a web standard technology, after all, if you want people to adopt System.Text.Json over NewtonSoft you're going to address people's demands not just ignore them and say it's on the backlog.

@payoff
Copy link

payoff commented Mar 1, 2023

@davidfowl any plan to release in .NET 8?

@lennieh
Copy link

lennieh commented Jun 1, 2023

Minimal API in .Net 7 supports patch operations. Has anyone worked out how to use this with JsonPatchDocument??

@sulmar
Copy link

sulmar commented Jun 1, 2023

You can do it, step by step, this way, for example:

dotnet add package Newtonsoft.Json
app.MapPatch("/", async (HttpContext context) =>
{
    if (context.Request.HasJsonContentType())
    {       
        using var reader = new StreamReader(context.Request.Body);
        var json = await reader.ReadToEndAsync();
        var doc = JsonConvert.DeserializeObject<JsonPatchDocument>(json);

        var originalDocument = new Models.Document();  // change to get an entity from database
        doc.ApplyTo(originalDocument);

        return Results.Ok(originalDocument);        
    }
    return Results.BadRequest();
});


public class Document
{
    public string Baz { get; set; } = "qux";
    public string[] Hello { get; set; }
    public string Foo { get; set; } = "bar";
}

With this solution Newtonsoft.Json is used only for single endpoint.

Maybe someone knows a better solution?

@payoff
Copy link

payoff commented Jun 1, 2023

Maybe someone knows a better solution?

Probably use:
https://github.com/Havunen/SystemTextJsonPatch

@deleteLater
Copy link

For those who prefer to avoid adding the Microsoft.AspNetCore.Mvc.NewtonsoftJson package and an extra input formatter, as mentioned in the documentation, here's an alternative solution:

[HttpPatch]
public async Task<ApiResponse<bool>> PatchAsync([FromBody] JsonElement jsonElement)
{
    // Use Newtonsoft.Json.JsonConvert to deserialize the JSON string into a JsonPatchDocument
    var patch = JsonConvert.DeserializeObject<JsonPatchDocument>(jsonElement.GetRawText());
}

@sulmar
Copy link

sulmar commented Jun 1, 2023

Thank to @deleteLater we can use in Minimal Api:

app.MapPatch("/", async ([FromBody] JsonElement jsonElement) =>
{
      var json = jsonElement.GetRawText();

      var doc = JsonConvert.DeserializeObject<JsonPatchDocument>(json);

      var originalDocument = new JsonPatchDocumentMinimalApi.Models.Document();
      doc.ApplyTo(originalDocument);

      return Results.Ok(originalDocument);
});

@sulmar
Copy link

sulmar commented Jun 1, 2023

I'm looking for better implementation yet.

I thought about using my own implemention of IModelBinder but in the minimal api seems not to be supported.

In .NET 7 for Custom Binding we can use only TryParse or BindAsync methods:
https://learn.microsoft.com/en-us/aspnet/core/fundamentals/minimal-apis/parameter-binding?view=aspnetcore-7.0#custom-binding

It is easy to add for own model but not for JsonPatchDocument.

@amcasey amcasey added the area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions label Jun 2, 2023
@captainsafia captainsafia added area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc and removed area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels labels Jun 20, 2023
@tjmcdonough
Copy link

@Havunen solution works very well. I have just given the GitHub project a star :) Highly recommend we all use this until Microsoft comes up with a solution

@zinov
Copy link

zinov commented Aug 17, 2023

I'm looking for better implementation yet.

I thought about using my own implemention of IModelBinder but in the minimal api seems not to be supported.

In .NET 7 for Custom Binding we can use only TryParse or BindAsync methods: https://learn.microsoft.com/en-us/aspnet/core/fundamentals/minimal-apis/parameter-binding?view=aspnetcore-7.0#custom-binding

It is easy to add for own model but not for JsonPatchDocument.

this is something that should be coming with the framework instead of doing a custom model binder to map all parameters. Microsoft should follow the standard: https://datatracker.ietf.org/doc/html/rfc6902

@Arkovski
Copy link

Arkovski commented Apr 17, 2024

mkArtakMSFT you're thinking quite a lot, maybe time to do anything? :)

@rohitvipin
Copy link

Happy 4 year anniversary for a very important issue :(

@DanielThyselius
Copy link

Bugs and requests against it just seem to get backlogged (including two of mine) despite very little activity in that area. I think JsonPatch sharing a backlog with core ASP.NET Core means it'll always be deprioritised unless a conscious push is made

Can you try out SystemTextJsonPatch? I believe I have fixed both the issues you reported about AspNetCore.JsonPatch. I know its not the official Microsoft package but personally I just needed a package that works and does not depend on Newtonsoft.

I've tried this out and it worked out of the box, brilliant work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affected-few This issue impacts only small number of customers area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-json-patch severity-minor This label is used by an internal tool
Projects
None yet
Development

No branches or pull requests