Graphql-dotnet: Use System.Text.Json for serialization/deserialization

[johnrutherford]

https://www.nuget.org/packages/System.Text.Json

Remove JSON.NET dependency. Make JSON.NET support a separate, optional package.

This is the direction that ASP.NET Core is going for performance so I thought I'd throw it out there. Though I don't know when System.Text.Json will be ready for the 3.0 time frame.

[sungam3r]

Yes, the thought is good. Now there is a broad tendency to take away this package.

[DaniilSokolyuk]

Blocked by https://github.com/dotnet/corefx/issues/39808
Dictionary null values are serialized as empty objects in preview7

[MerickOWA]

This appears to be fixed in core 3.1, any chance this can move forward now?

[sungam3r]

@joemcbride I propose to consider the option of taking serialization to abstraction, something like interface ISerializer. The graphql-dotnet package will no longer depend on Newtonsoft.Json. We will have to make 2 additional packages - like GraphQL.Serialization.NewtonsoftJson and GraphQL.Serialization.SystemTextJson. And the developer will have to install one or another package. This is a common practice of transferring things that are not specific to the main package to external packages. In ASP.NET Core this is done in a similar way - https://www.nuget.org/packages/Microsoft.AspNetCore.Mvc.NewtonsoftJson

So GraphQL will get rid of the only external dependency and all the problems associated with managing its versions. It will be possible to update specific serialization packages independently.

Generally speaking, the options are not limited to the two serializers mentioned above. For example, there is a Jil.

[sungam3r]

Also, move of the serializer in a separate package will allow us not to resort to multi-targeting in GraphQL-dotnet.

[joemcbride]

@sungam3r Agreed, I think that is a good approach. It would be good to see if we could somehow make it generic enough that we could still have the main logic in the core project but just offload the serialization part. That could get too far into the weeds, but could save some duplication.

[MerickOWA]

I opened a pull request to try and help move this along. I extracted out the Newtonsoft specific APIs into the GraphML.Serialization.Newtonsoft. I was able to get all the unit tests to pass once I added the reference to the new library and added a hack for the missing newtonsoft attributes on the ExecutionResult class.

[MerickOWA]

A more generic SchemaExtensions.ExecuteAsync probably needs to be added which takes an IDocumentWriter rather than relying on creating an internal one.

[sungam3r]

@MerickOWA Your PR does not contain the most important element of the puzzle - serialization abstraction.

A more generic SchemaExtensions.ExecuteAsync probably needs to be added which takes an IDocumentWriter rather than relying on creating an internal one.

You can use new property in ExecutionOptions : ExecutionOptions.Serializer. No need to move/copy entire classes into new package. Сut the code more carefully :)

[sungam3r]

I look at SchemaExtensions one more time and realized that it is used only in tests. So we can just move this class into test project, not in new Json propject.

[sungam3r]

Regarding my comment:

Your PR does not contain the most important element of the puzzle - serialization abstraction.

I realized that I was wrong. In this project such an abstraction is not needed, it is enough to have IDocumentWriter. Such an abstraction is required in the server project. It also depends on Newtonsoft.Json and there we also have to separate code. See, for example, how it uses ToInputs method.

[joemcbride]

Such an abstraction is required in the server project.

@sungam3r I would say that feels like an example of there being too much of a difference between the projects. What can we do in the core project to make it easier?

[sungam3r]

Well, I would not say that there are some difficulties. It's just that the process of removing a dependency affects not 1, but 2 repositories, that's all. This is a cross-cutting dependency for two repositories. Ordinary situation. I will return with details later.

[MerickOWA]

I did run across another issue in trying to implement the System.Text.Json serializers/deserializers. The default implementation of these serializers don't include support for System.Numeric.BigInteger. Its possible to implement JsonConverter to add support to read, but currently there's no way to implement the writting part as currently the necessary API is internal and not public.

See https://github.com/dotnet/corefx/issues/39314

[sungam3r]

Well, we are not required to provide converters for all possible types. After all, customers can have their own scalars and their own converters.

[benmccallum]

Awesome work @MerickOWA . Are you going to PR this back? I'm keen to get this in before we swap over to 3.1 in our prod env.

[benmccallum]

Sorry, just read your thread above and it does seem like your intent is to make this a PR. Awesome. Shout out if you need any help.

[sungam3r]

@benmccallum System.Text.Json support is a separate task - #1449

[sungam3r]

Fixed by #1512 , thanks to @benmccallum.

[dirkesquire]

I tried copying the code from GraphQL.Harness.GraphQLMiddleware to switch to using System.Text.Json, and I read the README but I keep getting: System.Text.Json.JsonException: '0x0A' is invalid within a JSON string. The string should be correctly escaped.
This was working perfectly fine with Newtonsoft.Json, but obviously I am trying to transition off of that. Have any of you come across this by chance?

These are my JsonSerializerOptions:

var request = await JsonSerializer.DeserializeAsync<GraphQLRequest>
            (
                context.Request.Body,
                new JsonSerializerOptions { 
                    PropertyNameCaseInsensitive = true,
                    ReadCommentHandling = JsonCommentHandling.Skip,
                    AllowTrailingCommas = true
                }
            );

This is my Dependency Injection:
services.AddTransient<IDocumentWriter, GraphQL.SystemTextJson.DocumentWriter>();

I am using: GraphQL.SystemTextJson -v 3.0.0-preview-1490 and dotnet 3.1.101

[sungam3r]

I think you have "malformed" json text. S.T.J is very strict about allowed characters. Newtonsoft.Json in this respect allows more freedom. Look into incoming request text (logs).

[dirkesquire]

Thanks so much. I'll look into that. I'm reading data straight from Microsoft.EntityFramework 3.1.2 (from a Azure SQL database). so I I'm not sure what would possibly be malforming it. 0x0A appears to be a newline character apparently. I don't see any JsonSerializerOptions regards to new lines unfortunately.

[sungam3r]

It is not something handled by JsonSerializerOptions. See https://stackoverflow.com/questions/42068/how-do-i-handle-newlines-in-json