Async request stream deserialization with System.Text.Json

[benmccallum]

This PR:

1. Abstracts the deserialization of GraphQLRequest behind an interface and provides two implementations; one for legacy (using Newtonsoft.Json) and one using System.Text.Json as the default for netcoreapp3.0+ targets.
2. With the SystemTextJson serializer, the request body is deserialized asynchronously, therefore avoiding the issue in netcoreapp3.0+ where the framework throws on the legacy, synchronous deserialization unless AllowSychronousIO is configured to true.
3. Also upgrades to use downstream updates in GraphQL repo to solve the same sync issue when writing to the response stream in netcoreapp3.0+

[benmccallum]

Bloody hell, how do you make a PR a draft PR once it's not. Sigh. Anyway... this is a WIP.

I'm kinda thinking this might be all that's required to avoid the IIS/Kestrel setting as it's probably just choking on the stream of the request body being read syncly, but I'll find out soon enough.

Oustanding:

1. Peek issue - need an async equivalent or a diff approach here.
2. [JsonPropertyName] doesn't seem to work... pretty sure I'm doing it right... annoying.

[sungam3r]

Thanks for step forward. For me, the main question is whether to support another serializer in-place or with external package. You have now linked the choice of serializer to target platform. What if I prefer to use old serializer in netcore3.0 app ? I may want to do this due to some specific settings available to me only in Newtonsoft.Json. Or if I really want to use a completely different serializer (Jil) ?

Also regarding graphql-dotnet/graphql-dotnet#1448. After merging it we should decide how to deal with APIs like public static IGraphQLBuilder AddGraphQL(this IServiceCollection services, Func<IServiceProvider, GraphQLOptions> options) which refers Newtonsoft.Json. Will it be configurable through delegates or just other #if / #else switches ? I have not studied this question carefully yet.

[sungam3r]

I am considering this option (message text is approximate):

      public static IGraphQLBuilder AddGraphQL(this IServiceCollection services, Func<IServiceProvider, GraphQLOptions> options)
        {
            services.TryAddSingleton<IDocumentExecuter, DocumentExecuter>();
            services.TryAddTransient(typeof(IGraphQLExecuter<>), typeof(DefaultGraphQLExecuter<>));
            services.AddSingleton(p => Options.Create(options(p)));

            services.TryAddSingleton<IDocumentWriter>(x =>
            {
                throw new InvalidOperationException("IDocumentWriter not set in DI. Add one of IDocumentWriter implementation, for example GraphQL.NewtonsoftJson.DocumentWriter or GraphQL.SystemTextJson.DocumentWriter");
            });

            return new GraphQLBuilder(services);
        }

Thus, we provide freedom of action to the caller. We have no dependency on serializer packages. But at the same time, we retain (and clearly indicate) the need for at least some kind of implementation to work further.

[benmccallum]

@sungam3r , I've got this working now and passing tests.

You're right, we're taking on a specific serializer for netcoreapp3.0+ and not giving people choice. I'm not sure if that's a big deal though for this PR, where I'm only fixing up the sync read. My reasoning: it's not like the deserialization needed changes. We're adhering to the GraphQL spec, which is camelCase JSON.

So, do we really need to provide consumers with options of a serializer?

• If the GraphQL spec supported any serialization format (BSON, protobug, etc) both in the request and/or the response, I'd say yea, it'd be good to have interfaces and several different packages that give users choice of serialization methods, so their clients could call in differently, but that's not the case; it's really fixed to JSON.
• Another reason might be if they want control of the transitive dependencies, but with netcoreapp3.0+, the shared framework includes System.Text.Json so you're not pulling in another serializer you didn't already have.

I'm thinking really we should be the ones making the choice here, which essentially boils down to which is the fastest / most supported / easiest to maintain long-term.

What do you think @joemcbride?

[sungam3r]

and not giving people choice.

That is, to step on the same rake as it was with Newtonsoft.Json ;)

but that's not the case; it's really fixed to JSON

JSON is just a format, Newtonsoft.Json / System.Text.Json / whatever - implementations. You can use any implementation to work with this format.

it's not like the deserialization needed changes

Why so? This PR essentially claims the opposite. There was one implementation with one set of characteristics; another implementation with a different set of characteristics became.

We're adhering to the GraphQL spec, which is camelCase JSON.

Yes, but what follows from this? The specification does not say anything about how deserialization should occur.

I'm thinking really we should be the ones making the choice here, which essentially boils down to which is the fastest / most supported / easiest to maintain long-term.

The best strategy of all time in such cases when you need to choose the fastest / most effective / best solution is not to choose it at all. No need to solve this problem. You just need to put it out of the bracket, giving the choice to the caller. The caller will be more than happy with the opportunity to choose among several out-of-the-box (standard) implementations and the opportunity to create his own.

So, do we really need to provide consumers with options of a serializer?

My intuition says YES.

[benmccallum]

Gonna call it a night. Hopefully this has set us up along a decent path though.

Can you have a think about how you'd want the tests amd Samples.Server Startup to look like? Now that we've got two different pluggable serializers, I guess we need to make sure we're testing them. I'm thinking we can just pass through a config value from the tests that tells the Startup to toggle between deserializers. Should be easy enough.

[sungam3r]

I'm thinking we can just pass through a config value from the tests that tells the Startup to toggle between deserializers. Should be easy enough.

Ok. Also you can add command args in profiles for serializers in launchSettings.json.

[sungam3r]

I again looked at the changed files. As I understand it, there are several cosmetic changes left in the documentation and code. In addition, you were going to do more tests.

[sungam3r]

Also please look at graphql-dotnet/graphql-dotnet#1541

[benmccallum]

LGTM :)

[benmccallum]

Actually wait, I think we can now add Graph type to the test suite too now.

[benmccallum]

OK, added. All yours. Surely this is it! LGTM!!

[sungam3r]

I think it's time to merge. This is necessary at least for synchronization with changes in core package. In any case, the changes are repeatedly verified.

[benmccallum]

Let's do it! I've got a branch off this one where I was playing with a benchmark so we can see what the go is. Will rebase it off develop though once this is merged.

[benmccallum]

🎉