Telegram.Bot icon indicating copy to clipboard operation
Telegram.Bot copied to clipboard

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

Open poulad opened this issue 6 years ago • 18 comments

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 avatar Oct 08 '19 12:10 poulad

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 avatar Oct 08 '19 13:10 poulad

@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.

https://github.com/TelegramBots/Telegram.Bot/pull/838#discussion_r332608326

poulad avatar Oct 08 '19 22:10 poulad

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

dumkin avatar Nov 05 '21 12:11 dumkin

@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.

tuscen avatar Nov 05 '21 12:11 tuscen

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 avatar Nov 05 '21 18:11 dumkin

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

dumkin avatar Nov 05 '21 18:11 dumkin

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

tuscen avatar Nov 05 '21 19:11 tuscen

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

peter-mghendi avatar Mar 10 '22 14:03 peter-mghendi

@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

tuscen avatar Mar 10 '22 18:03 tuscen

I think things are changed in .net 8

Lonli-Lokli avatar Aug 07 '23 18:08 Lonli-Lokli

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

klinki avatar Oct 17 '23 15:10 klinki

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 )

Fedorus avatar Oct 17 '23 15:10 Fedorus

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.

klinki avatar Oct 17 '23 15:10 klinki

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

luizfbicalho avatar Feb 24 '24 22:02 luizfbicalho

@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.

tuscen avatar Feb 24 '24 23:02 tuscen

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

luizfbicalho avatar Feb 24 '24 23:02 luizfbicalho

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 avatar Feb 24 '24 23:02 tuscen

@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 :)

tuscen avatar Feb 24 '24 23:02 tuscen