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

Replace Newtonsoft with System.Text.Json #517

Closed
ghost opened this issue May 13, 2020 · 44 comments
Closed

Replace Newtonsoft with System.Text.Json #517

ghost opened this issue May 13, 2020 · 44 comments

Comments

@ghost
Copy link

ghost commented May 13, 2020

NOTE: Although this was a very popular request and a cornerstone of 4.0, there are no doubt many people who would prefer to continue using the Newtonsoft-backed serializer. A companion library has been released for this purpose.


Please describe the feature/enhancement in as much detail as possible.
Recent Microsoft documents show some advantages of the System.Text.Json library such as high performance ... Although I do not deny that Newtonsoft.Json is still too good and popular, however We can replace it for a better Flurl library, limiting the dependencies of external packages

@ghost ghost added the enhancement label May 13, 2020
@maxkatz6
Copy link

Note that Flurl support net45;net46;netstandard1.1;netstandard1.3, which should be dropped to support System.Text.Json. Or use two json libs in same time for different targets.

@imtrobin
Copy link

No, there are still limitations with MS Json compared to newtonsoft

@ghost
Copy link
Author

ghost commented May 13, 2020

I have found an ISerialize interface, so can I implement members of this interface when using with System.Text.Json?

@tmenier
Copy link
Owner

tmenier commented May 13, 2020

I've heard this request before and there's definitely interest in it, but I agree with all the drawbacks as well. Aside from platform support, I'm worried I'd break a lot of people who use Json.NET's serialization attributes, global settings, etc. To do it right would probably mean releasing multiple packages. For now, I don't see the payoff being worthwhile. But I could see that changing at some point. I'll keep this issue open as a placeholder.

@snekbaev
Copy link

@tmenier agreed, no need for surprises related to serialization/deserialization for now :)

@tmenier
Copy link
Owner

tmenier commented May 13, 2020

@mincasoft Yes, you can definitely swap out Newtonsoft for System.Text.Json today: https://flurl.dev/docs/configuration/#serializers

For most people though, I'm pretty sure the motivation is to eliminate the Newtonsoft dependency and/or reduce the payload size. This doesn't accomplish that, but it's a way to use the MS's lib if you want to.

@rhyland
Copy link

rhyland commented May 13, 2020

Please leave Newtonsoft, as there is not enough support in Core 3.1, yet. Thanks.

@andrewdmay
Copy link

Using Flurl at enterprises has been difficult because of the Newtonsoft dependency being newer than the version they have been using for years in other applications and a fear of upgrades.
A version that only depended upon System.Text.Json might be a way to avoid that dependency issue (even if it means that they have two JSON libraries instead of one).

@tmenier
Copy link
Owner

tmenier commented Sep 12, 2020

I'm confident this will happen in future major release, but it isn't gonna make the 3.0 cut. I could see an optional Flurl.Http.Newtonsoft package or something for those who want to stick with Json.NET.

@tmenier tmenier added the 4.0 label Sep 12, 2020
@bonesoul
Copy link

+1 for the change.

@rcollette
Copy link

There are a couple "warnings" I see regarding the implementation suggested in #517 (comment)

Screen Shot 2020-12-11 at 1 54 20 PM

First is the fact that JsonSerializer.Deserialize methods are nullable but the ISerializable methods are not. Are we OK using the nullability forgiveness operator in this case?

Second is the fact that stream deserialization is asynchronous with JsonSerializer but not in ISerializable. Retrieving the Result of a ValueTask is not guaranteed to contain a value. Are we safe retrieving Result in this manner?

@panoukos41
Copy link

I came across this issue when I was searching for System.Text.Json, maybe include the following class in the library or as an extra package? It works on my application as expected.

public class SystemJsonSerializer : ISerializer
{
    private readonly JsonSerializerOptions? _options;

    public SystemJsonSerializer(JsonSerializerOptions? options = null)
    {
        _options = options;
    }

    public T Deserialize<T>(string s)
    {
        return JsonSerializer.Deserialize<T>(s, _options)!;
    }

    public T Deserialize<T>(Stream stream)
    {
        using var reader = new StreamReader(stream);
        return Deserialize<T>(reader.ReadToEnd());
    }

    public string Serialize(object obj)
    {
        return JsonSerializer.Serialize(obj, _options);
    }
}

@tmenier
Copy link
Owner

tmenier commented Apr 18, 2021

@panoukos41 Thanks, good to know that works. Hopefully others who want to use System.Text.Json today will find that useful.

@benwmills
Copy link

The mix is getting odd when developing in .NET 5. I built my POCOs with [JsonProperty] to support using Flurl, but now I want to use the same POCO for an incoming webhook and .NET 5 binding won't work.

I can add [JsonProperty] and [JsonPropertyName] to each property in the POCO, but that is a little icky.

I can avoid the binding and deserialize manually using JSON.NET.

I can also force ASP.NET to use JSON.NET across the board.

None of these options feel right. I've loved JSON.NET for a long time, but it seems like System.Text.Json is the future at some point.

I think this has been said, but ideally, Flurl wouldn't have the dependency on JSON.NET, but adding a separate bolt on package would make Flurl function as it does today.

@tmenier
Copy link
Owner

tmenier commented May 27, 2021

@benwmills You can also swap out the (default) Newtonsoft serializer for this one, which I think is a better option in your case than any of those you mentioned.

@jrgcubano
Copy link

jrgcubano commented Jun 10, 2021

We have created a package with the System.Text.Json serializer to simplify our life and for those who don't want to copy/paste all the time. https://github.com/YellowLineParking/Appy.Flurl

Thanks @panoukos41 for the code and @tmenier for the library.

@WatchDogsDev
Copy link

No
Noooooooooooooooo

@ItsDaveB
Copy link

The mix is getting odd when developing in .NET 5. I built my POCOs with [JsonProperty] to support using Flurl, but now I want to use the same POCO for an incoming webhook and .NET 5 binding won't work.

I can add [JsonProperty] and [JsonPropertyName] to each property in the POCO, but that is a little icky.

I can avoid the binding and deserialize manually using JSON.NET.

I can also force ASP.NET to use JSON.NET across the board.

None of these options feel right. I've loved JSON.NET for a long time, but it seems like System.Text.Json is the future at some point.

I think this has been said, but ideally, Flurl wouldn't have the dependency on JSON.NET, but adding a separate bolt on package would make Flurl function as it does today.

Exactly the same issue here, .NET 5. Mixing both packages is causing strange behaviour and I would favour using System.Text.Json out of the box instead of including other dependencies.

@chuac
Copy link

chuac commented Jan 28, 2022

If you're using the serializer above and are working with string representations of enums, you need to also pass in a JsonStringEnumConverter via JsonSerializerOptions.

Otherwise, you will get an exception: System.Text.Json.JsonException: The JSON value could not be converted to <Enum Type> when deserializing.

_options = new JsonSerializerOptions();
_options.Converters.Add(new JsonStringEnumConverter());

@Hawxy
Copy link

Hawxy commented May 7, 2022

Just a note that S.T.J includes an overload that accepts a stream in .NET 6, so the example above can be simplified down to:

public class SystemJsonSerializer : ISerializer
{
    private readonly JsonSerializerOptions? _options;

    public SystemJsonSerializer(JsonSerializerOptions? options = null)
    {
        _options = options;
    }

    public T Deserialize<T>(string s) => JsonSerializer.Deserialize<T>(s, _options)!;

    public T Deserialize<T>(Stream stream) => JsonSerializer.Deserialize<T>(stream, _options)!;

    public string Serialize(object obj) => JsonSerializer.Serialize(obj, _options);

}

@tmenier
Copy link
Owner

tmenier commented May 27, 2022

My plan is to swap out Newtonsoft for System.Text.Json and release as 4.0-pre1 relatively soon. It should be considered reasonably stable since it will consist of virtually no other changes from the final 3.x. But if you're still not comfortable using a prerelease version, you'll need to wait a bit longer for me to break a few more things in follow-up prereleases before the final 4.0 :). Sound like a reasonable plan?

The only other thing I'm considering for prerelease 1 (since it's also related to reducing footprint) is dropping the SourceLink dependency. Not really my area of expertise, but now that symbols packages are being published to NuGet I don't think we need it anymore. If you have thoughts one way or the other, please weigh in in #513.

@ItsDaveB
Copy link

I'd go with option 1. I've never used dynamic myself unless I absolutely had to (Office interop code). I just think it's ugly and unsafe.

Yes, it might remove the need to write a few small classes for complex nested JSON structures. But I never felt that it is worth the risk.

I agree dynamic should be avoided at all times if possible. Typing classes for nesting should be standard as expected.

@gitfool
Copy link
Contributor

gitfool commented May 28, 2022

Very interesting. After reading through the various issues quoted above I also concluded that dynamic should be avoided going forward. The price of this syntactic sugar is too high and there are, albeit more verbose and ugly, alternatives available.

Also, bearing in mind that you're bumping the major version to signal a breaking change by moving from Newtonsoft.Json to System.Text.Json, this is the best time to make a clean break.

@tmenier tmenier changed the title Should we replace the Newtonsoft.Json library with System.Text.Json? Replace Newtonsoft with System.Text.Json May 29, 2022
@tmenier
Copy link
Owner

tmenier commented Jun 3, 2022

Ok, you asked for it, dynamic returning methods in Flurl are getting the 🪓! #699

@tmenier
Copy link
Owner

tmenier commented Jun 3, 2022

I bumped into another issue with STJ. If you attempt to deserialize an empty response stream (or empty string) to any nullable type, you get this exception:

System.Text.Json.JsonException : The input does not contain any JSON tokens. Expected the input to start with a valid JSON token, when isFinalBlock is true.

That's different than how Newtonsoft behaves. In my mind Newtonsoft has it right. In both cases the absence of a JSON property will deserialize to a null (or default) value in the C# object by default, so why should the absence of the entire document behave any differently?

I updated the serializer to return default(T) for empty streams and strings and everything's now working as I'd expect. Does everyone agree with this decision? If so, I believe this was final hurdle to releasing this.

tmenier added a commit that referenced this issue Jun 3, 2022
@cremor
Copy link

cremor commented Jun 4, 2022

I agree that returning default is better than throwing exceptions in this case.

@tmenier
Copy link
Owner

tmenier commented Jun 4, 2022

Here's a Newtonsoft-based serializer for those who prefer not to switch:

https://gist.github.com/tmenier/158ede5f33703036a0c26080adf2b8b9

@tmenier
Copy link
Owner

tmenier commented Jun 5, 2022

The first 4.0 prerelease is available! If you've been waiting for the switch to STJ, I HIGHLY encourage you to test it out. (Conversely, if you prefer to stick with Newtonsoft, there's not much in this initial prerelease for you 😁 )

@ZombiesWithCoffee
Copy link

So far, so good on the testing of 4.0!
I am now going thru and change all my DataMember attributes to JsonPropertyName.

When you get a chance, can you write up write up on how to add System.Text.Json converters to your global Flurl settings?
For now, I'm returning a text string, then converting it.

            var text = await request
                .AppendPathSegments("test", value)
                .PutJsonAsync(new TPageModel())
                .ReceiveString();

            var serializeOptions = new JsonSerializerOptions();

            serializeOptions.Converters.Add(new Int32Converter());
            serializeOptions.Converters.Add(new Int16Converter());
            serializeOptions.Converters.Add(new Int16NullableConverter());
            serializeOptions.Converters.Add(new GuidConverter());

            return JsonSerializer.Deserialize<TPageModel>(text, serializeOptions);

@tmenier
Copy link
Owner

tmenier commented Jun 8, 2022

@ZombiesWithCoffee The pattern is very similar to how you would provide custom JsonSerializerSettings with Newtonsoft. That is, the new STJ-based serializer (just called DefaultJsonSerializer) optionally take a JsonSerializerOptions constructor arg. Build the options like you did and apply it globally like this:

FlurlHttp.Configure(settings => settings.JsonSerializer = new DefaultJsonSerializer(jsonSettings));

Or apply it to a client or individual request if you don't want to mess with the global context.

(I'll update the docs when 4.0 gets closer to full release, thanks for the reminder on that.)

@ZombiesWithCoffee
Copy link

That worked beautifully! Thanks @tmenier

@ZombiesWithCoffee
Copy link

Question @tmenier - I'm having to add a 'JsonPropertyName("FirstName")' to every model that I'm serializing thru flurl with this System.Text.Json change.

Is there a global property I'm not setting? Or is it now required to add?

@tmenier
Copy link
Owner

tmenier commented Jun 8, 2022

@ZombiesWithCoffee What Flurl is doing is pretty simple and all tests are passing without any custom options. I don't know why you're seeing what you're seeing but I don't think it's a Flurl problem. Maybe some of your custom settings are squashing some default behavior or something.

@ZombiesWithCoffee
Copy link

ZombiesWithCoffee commented Jun 8, 2022

I agree, thanks for the confirmation. I've used your product since v1. I'm searching for why all my flurl calls are suddenly returning values without the property being set, but so far, every call is returning null for all properties.

Sadly, after several hours of attempting to find the issue, I'm going to have to revert back to Newtonsoft for now.

@Hawxy
Copy link

Hawxy commented Jun 9, 2022

I'm searching for why all my flurl calls are suddenly returning values without the property being set, but so far, every call is returning null for all properties.

If you'd like STJ behavior a little closer to Newtonsoft, I'd recommend setting:

new JsonSerializerOptions(JsonSerializerDefaults.Web)

It makes property names case-insensitive, among a few other things.

@tmenier
Copy link
Owner

tmenier commented Jun 20, 2022

Quick note that prerelease 2 is now available. Nothing exciting or specific to this issue, but more breaking changes you might want to keep up with.

@adam-knights
Copy link

The pre release worked really well for us, will eagerly await 4.0's full release :).

Just a note for those with issues, one of the key settings will be

var options = new JsonSerializerOptions
{
    PropertyNameCaseInsensitive = true
};

(or use JsonSerializerDefaults.Web as Hawxy mentions above)

as detailed at https://docs.microsoft.com/en-us/dotnet/standard/serialization/system-text-json-character-casing. Newtonsoft was case insensitive by default whereas STJ is sensitive by default (resulting in your properties not populating if you had differences previously and don't set this).

Another useful difference between NS and STJ to know is around being tolerant of integers as strings in the json - https://docs.microsoft.com/en-us/dotnet/standard/serialization/system-text-json-migrate-from-newtonsoft-how-to?pivots=dotnet-6-0#allow-or-write-numbers-in-quotes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Released
Development

No branches or pull requests