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

Support System.Text.Json (as well as Newtonsoft.Json) #839

Open
poulad opened this issue Oct 8, 2019 · 18 comments · May be fixed by #1078
Open

Support System.Text.Json (as well as Newtonsoft.Json) #839

poulad opened this issue Oct 8, 2019 · 18 comments · May be fixed by #1078

Comments

@poulad
Copy link
Member

poulad commented Oct 8, 2019

We can use new and high performance JSON serialization System.Text.Json package. Faster serialization/deserialization of request and response data structures is the main reason for this suggestion.

There are a few things to consider:

System.Text.Json requires .NET Standard 2.0 and that's .NET 4.6.1+.

Snake case naming policy support won't come until .NET 5 (see dotnet/corefx#41354). Alternatives are using camel cased naming policy by default, e.g. for username, and overriding specific fields, e.g. for first_name, with the [JsonPropertyName("first_name")].

Custom type converters such as for DateTime to Unix epoch time need to be implemented. This is very similar to the current implementation for Newtonsoft.Json.

References

The future of JSON in .NET Core 3.0

@poulad
Copy link
Member Author

poulad commented Oct 8, 2019

Ignoring Default Values

As @tuscen pointed out on #838, default value of the value types should be ignored during serialization but it's not supported at the moment. Default null value of the reference types can be ignored using the IgnoreNullValues in serialization options.

class Foo
{
    [JsonPropertyName("field_bar")]
    public bool FieldBar { get; set; }
    
    [JsonPropertyName("field_baz")]
    public string FieldBaz { get; set; }
}
[Fact]
public void Test()
{
    var opts = new JsonSerializerOptions { IgnoreNullValues = true };
    string json = JsonSerializer.Serialize(new Foo(), opts);
    // JSON is {"field_bar":false}

    // assertion fails
    Assert.Equal("{}", json);
}

see more at dotnet/corefx#40922

@poulad
Copy link
Member Author

poulad commented Oct 8, 2019

@tuscen :

...webhook users will need to set custom JsonSerializerOptions specifically for Bot API, because these options are global for the entire middleware pipeline in ASP.NET Core. If I'd want to host telegram webhook in the same process as my web app options needed for the library might not be compatible with the app. With the current setup we don't even need custom serializer options.

#838 (comment)

@dumkin
Copy link

dumkin commented Nov 5, 2021

Ignoring Default Values

As @tuscen pointed out on #838, default value of the value types should be ignored during serialization but it's not supported at the moment. Default null value of the reference types can be ignored using the IgnoreNullValues in serialization options.

class Foo
{
    [JsonPropertyName("field_bar")]
    public bool FieldBar { get; set; }
    
    [JsonPropertyName("field_baz")]
    public string FieldBaz { get; set; }
}
[Fact]
public void Test()
{
    var opts = new JsonSerializerOptions { IgnoreNullValues = true };
    string json = JsonSerializer.Serialize(new Foo(), opts);
    // JSON is {"field_bar":false}

    // assertion fails
    Assert.Equal("{}", json);
}

see more at dotnet/corefx#40922

dotnet 5&6 can do that now
image

@tuscen
Copy link
Member

tuscen commented Nov 5, 2021

@dumkin unfortunately this is not a solution because if someone would use the library with a webhook they could need different json options compared to what we need. And as far as I know json options are global in ASP.NET Core.

@dumkin
Copy link

dumkin commented Nov 5, 2021

I don't really understand at all why it is necessary to create a controller with an action with parameters. Why not just ask to add an additional route. And get everything you need from the body. And here it doesn't matter. use system.text.json or newtonsoft.json. it will not affect other libraries and user code

example for webhook
image

@dumkin
Copy link

dumkin commented Nov 5, 2021

It's just that now, in essence, the situation is the same. Only the other way around. now it is required

services.AddControllers()
                    .AddNewtonsoftJson();

Which forces us to use NewtonsoftJson exactly as globally

@tuscen
Copy link
Member

tuscen commented Nov 5, 2021

You have a point 🤔 We'll think about it.

@peter-mghendi
Copy link

This is something I would very much like in this library. Is there anything I can help with that would get this done quicker?

@tuscen
Copy link
Member

tuscen commented Mar 10, 2022

@sixpeteunder Unfortunately , there's not much you can do. We rely heavily on:

  • completely attribute-based configuration (no custom JsonSerializerSettings), but that feature we might be ok with dropping
  • opt-in property strategy, it's safer because probability of accidental adding of the json attribute on a property is lower than ensuring that all ignored properties are annotated with [JsonIgnore]
  • built-in snake case property conversion
  • Required properties validation during (de-)serialisation

All these features are still missing even in .NET 6.

Basically STJ is still mostly in MVP stage and is usable for APIs you control completely

@Lonli-Lokli
Copy link

I think things are changed in .net 8

@klinki
Copy link

klinki commented Oct 17, 2023

This would be really great to have, it is a blocker for AOT.

@Fedorus
Copy link
Member

Fedorus commented Oct 17, 2023

This would be really great to have, it is a blocker for AOT.

Well, usage in Unity or front end blazor is strongly not recommended, so its not big of a deal )

@klinki
Copy link

klinki commented Oct 17, 2023

True, but AOT is not just about raw performance but also about memory footprint. Especially trimming is really great to reduce memory footprint. I'm experimenting with TelegramBot client used for forex trading running on very low spec VPS provided by my forex broker. It has just 1GB RAM available and it has to run MT4 platform + my telegram bot. Currently it works, but sometimes I'm getting near the memory limit.

@luizfbicalho
Copy link

Whats the status on this? looks like there is a lot of problems to migrate

@tuscen
Copy link
Member

tuscen commented Feb 24, 2024

@luizfbicalho All the changes are in the branch tuscen/stj-2 and I'd say it's 80-90% ready. I just don't have enough free time to squash all the bugs and finish it. I want to release Bot API 7.0 and 7.1 updates first and then release STJ support.

The thing is that it ended up quite non-trivial in a lot of places and it probably won't be backwards compatible so it'll be released as a new major version. The current plan is to completely remove Newtonsoft from net7.0 and newer targets and use built-in STJ from the runtime. All the targets below net7.0 will stay on Newtonsoft because earlier versions of STJ don't have some features that we use from Newtonsoft.

@luizfbicalho
Copy link

I have the same issues with my projects, thanks for the answer and I'll see it if I can help in any way

@tuscen
Copy link
Member

tuscen commented Feb 24, 2024

Also the first release with STJ will not be AOT compatible. I want to implement it in a separate release, because otherwise it will take too much time.

@tuscen
Copy link
Member

tuscen commented Feb 24, 2024

@luizfbicalho you can run unit tests from that branch and see which ones are failing. We also will need to add a few hundreds of serialization tests more to ensure that everything works as expected :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants