graphql-dotnet / graphql-client

A GraphQL Client for .NET Standard
MIT License
625 stars 134 forks source link

Request to add API that returns string responses/errors rather than deserialized results #139

Open sidhants opened 5 years ago

sidhants commented 5 years ago

This will help in making the library usable with il2cpp/no dynamics/WebGL based use cases including Unity which is a big community. The consumer of the API can perform the deserialized operation on their side as needed with any compatible package.

A small addition to the source code at my end was enough to get the responses to work nicely- public Task<string> SendQueryAsync(GraphQLRequest request, CancellationToken cancellationToken = default) => this.graphQLHttpHandler.PostAsync(request, cancellationToken);

il2cpp successful results in Unity- image

@deinok Please let me know what you think about this? I feel it can be very useful to increase the scope of use. Serialization does not have any issues currently with il2cpp once newton json is configured to turn off dynamics and works out of the box as expected. But deserialization does not work because of how the graphql response class is set up and dependent on dynamics data. Other idea would be to use some generic JsonConstructor in the graphql response class that prevents dynamics usage. The version I am using is 'GraphQL.Client.2.0.0-alpha.3'.

rose-a commented 4 years ago

@sidhants please check the current API (develop branch), it uses System.Text.Json instead of Json.Net and does not use dynamics anymore.

sidhants commented 4 years ago

@sidhants please check the current API (develop branch), it uses System.Text.Json instead of Json.Net and does not use dynamics anymore.

@rose-a I am going to give it another shot with Unity integration this weekend with the latest dev. Though I have heard System.Text.Json is not compatible with Unity probably due to reflection features not supported on il2cpp/iOS/VR/AR headsets. I will confirm this, otherwise need to think of another solution.

rose-a commented 4 years ago

@sidhants the biggest problem I discovered are the Extensions fields on GraphQLResponse and GraphQLError.

I'm not keen on having to specify 3 types for every request...

rose-a commented 4 years ago

Changed the type of extensions to System.Text.Json.JsonElement in e1c8f3b1ac5ffbfcec9dbf6d36f1aad6bcacf65b.

If the issue is just the dynamics this should solve it.

DamianMehers commented 4 years ago

System.Text.Json is also not currently supported by Xamarin iOS (or WatchOS) so returning raw JSON would help there too https://github.com/dotnet/runtime/issues/31326 .. I could deserialize using SimpleJson for example.

rose-a commented 4 years ago

Maybe a dumb question, but how does the serialization work if the serializer is not supported?

Also with websockets this won't be possible, they need to deserialize the response to match the request ID...

How do you do JSON serialization/deserialization on those platforms anyway?

sidhants commented 4 years ago

Maybe a dumb question, but how does the serialization work if the serializer is not supported?

Also with websockets this won't be possible, they need to deserialize the response to match the request ID...

How do you do JSON serialization/deserialization on those platforms anyway?

Well, this is the specific version compatible with Unity (Unity is multi platform to target android/ios/webgl/etc) https://github.com/jilleJr/Newtonsoft.Json-for-Unity. I had recompiled the repo with it. Serialization worked with this with the previous code and http requests were good, but response deserialization which used a dynamic data field in the graphql response class would make it crash on the device. Thus the raw response was a workable solution. Also, Unity has its own inbuilt JSON library https://docs.unity3d.com/ScriptReference/JsonUtility.html SimpleJSON also works as far as I know.

rose-a commented 4 years ago

So I suppose deserialization would also work when no dynamic fields would be used? Like deserializing some json into a JToken field (or JsonElement in case of 'System.Text.Json')?

I guess it would be possible to define an interface which provides the necessary methods for serialization and deserialization and allow the implementation to be injected into GraphQLHttpClient...

If we can then declare a generic for the "unknown type" fields (JsonElement by default) you would be set without manually deserializing queries afterwards (and able to use the websocket).

rose-a commented 4 years ago

the downside would be that we wouldn't be able to use attributes like [JsonIgnore]or [JsonPropertyName] anymore...

sidhants commented 4 years ago

So I suppose deserialization would also work when no dynamic fields would be used? Like deserializing some json into a JToken field (or JsonElement in case of 'System.Text.Json')?

I guess it would be possible to define an interface which provides the necessary methods for serialization and deserialization and allow the implementation to be injected into GraphQLHttpClient...

If we can then declare a generic for the "unknown type" fields (JsonElement by default) you would be set without manually deserializing queries afterwards (and able to use the websocket).

Thats a very good suggestion and would make things flexible. I think the main concern is compatibility of System.Text.Json as there were a lot of dependencies that Unity or other platforms did not like. (I will give it one more shot though to rule it out) newtonjson still seemed to have good support for il2cpp platforms. is that a possibility to revive/use without dynamic features with the injected code idea?

rose-a commented 4 years ago

newtonjson still seemed to have good support for il2cpp platforms. is that a possibility to revive/use without dynamic features with the injected code idea?

Yeah that's what I'm thinking too... with newtonsoft at least I would personally be able to build custom converters to achieve the things which are currently done with those properties in the primitive classes...

Guess this is somehow possible with System.Text.Json too, but I didn't figure out yet how to just override its behaviour for one layer (say "making the data field always lowercase) and leave the rest to the default serializer...

@sidhants Please check if the deserialization of an arbitrary object back to a JToken works on Unity

sidhants commented 4 years ago

newtonjson still seemed to have good support for il2cpp platforms. is that a possibility to revive/use without dynamic features with the injected code idea?

Yeah that's what I'm thinking too... with newtonsoft at least I would personally be able to build custom converters to achieve the things which are currently done with those properties in the primitive classes...

Guess this is somehow possible with System.Text.Json too, but I didn't figure out yet how to just override its behaviour for one layer (say "making the data field always lowercase) and leave the rest to the default serializer...

@sidhants Please check if the deserialization of an arbitrary object back to a JToken works on Unity

Yes. Going to do some tests this weekend with the pre-release.

DamianMehers commented 4 years ago

There is a big issue with Newtonsoft and AOT compilation which renders the generated code way too big to be used in some circumstances (see the "The 126MB Watch app" here.)

So Newtonsoft would not work for me, but if it makes sense for the general case then I can do a raw HTTP POST of the GraphQL query and then parse the result myself using something like SimpleJson which is used in the pre-GraphQL GitHub APIs.

rose-a commented 4 years ago

@DamianMehers does SimpleJson have a common base type for deserialized stuff (like JToken or JsonElement)? What happens if you deserialize an arbitrary JSON object to object?

Im trying to get around adding a complete string-returning interface to GraphQLClient by exactly defining the return type (which should work with basicly every Json library).

The biggest problem for this are the "Extension" fields on both GraphQLResponse and GraphQLError, for they can be "a map of anything" (the spec says "The response map may also contain an entry with key extensions. This entry, if set, must have a map as its value.").

DamianMehers commented 4 years ago

@DamianMehers does SimpleJson have a common base type for deserialized stuff (like JToken or JsonElement)? What happens if you deserialize an arbitrary JSON object to object?

Im trying to get around adding a complete string-returning interface to GraphQLClient by exactly defining the return type (which should work with basicly every Json library).

The biggest problem for this are the "Extension" fields on both GraphQLResponse and GraphQLError, for they can be "a map of anything" (the spec says "The response map may also contain an entry with key extensions. This entry, if set, must have a map as its value.").

I believe it does but I think this is an edge-case and that sooner or later System.Text.JSon will be supported on Xamarin so I would not worry. I'll hand-POST/decode the response from my Apple Watch app for now.

sungam3r commented 4 years ago

A map of anything is Dictionary<string, object> with inner dictionaries.

rose-a commented 4 years ago

@sungam3r Yeah, but thats something you cannot easily teach a JSON deserializer (especially not all of them with a single solution)... this can in theory have unlimited levels of depth....

I'm currently thinking if it wouldn't be best to just remove the Extensions fields from GraphQLResponse and GraphQLError and allow a serialization interface implementation to return derived classes which then again contain extension fields to their ability or use case (propably best to leave them out for Xamarin use cases)... I guess you don't need them for most use cases anyway, but still they're part of the GraphQL spec.

sungam3r commented 4 years ago

In my implementation of GraphQL client I just allow to return raw json string.

sungam3r commented 4 years ago

There are consumers for whom this is the best option.

sungam3r commented 4 years ago

It seems that I myself was faced with the need to get an untyped result. After upgrading to version 3, I cannot read the result of the introspection request as plain json text.

BillBaird commented 4 years ago

As to getting untyped results for GraphQLResponse.Data, since everything I did in the 2.0 release worked that way, and I have quite a bit of code that works with untyped responses, I realized that I could get that behavior by using a type of dynamic, JObject, JsonElement, Map, or Dictionary<string, object>> depending on the serializer used and how I wanted to work with the response. So, the generic GraphQLResponse<T> works fine for me (I typically just use dynamic) for untyped responses. Another benefit of using dynamic, JObject, or JsonElement is that they work well for responses which simply return a primitive type, such as a string. No wrapping is necessary.

Still, having a interface (as suggested in #224) which exposes GraphQLResponse in an untyped way along with allowing for GraphQLError and Extensions to be handled in a common way would be quite useful and reduce client code complexity.

emceelovin commented 4 years ago

This is more useful than most people think. The option to have a high level typing should be optional and not forced. I would also prefer this. People shouldn't be forced up to something. It should be an option, and people should be able to write code to achieve this. Serialization/Deserialization isn't cheap. This will cause people to have to do it twice potentially. I, myself, am just forwarding JSON from a micro-service back to a client where it will be handled by TypeScript. I am not interested in the JSON at all on the gateway in the middle. As @sidhants said, It seems like it wouldn't be hard at all, and be very useful, to have a Task SendQueryAsync(...) as an option. Nobody is asking to get rid of the default member functions.

As far as how this would be handled over a WebSocketTransport, you would have to work with @joemcbride on that. As there is no metadata or any kind of properties you can tag along with a WebSocket frame. It's all inside of the OperationMessage payload. However, I was reading the WebSocket RFC, didn't get much out of it. Frames do have an extension property. The specification has reserved a few opcodes for extensions, and they just have to be negotiated on connection. Then you can add extension data to a WebSocket frame payload. That way, you wouldn't have to deal with the WHOLE payload to grab the ID out of the frame/JSON at all. You would know exactly where it is at in the payload. Avoiding a deserialization. Just a little bit of information I figured I would offer.

EDIT I am using GraphQL over a WebSocket transport, and after looking at the code, I would be happy to even have graphql-ws sub-protocol OperationMessage.Payload as a string only. If I could have just the payload string, that would 100% satisfy my needs.

IGraphQLResponse.Data.ToString() will be fine for now. Thank you guys for everything.

emceelovin commented 4 years ago

@rose-a , if you end up implementing the return as a string, can you offer a byte[] return as well? For the people who end up wanting to send the data over a socket. Since a UTF8.GetString(byte[]) will mean another copy/encoding operation. maybe byte[] GraphQLResponse.PayloadAsBytes().

sungam3r commented 4 years ago

If you are worried about copy/encoding operations, then you can ask to provide the result as a stream.

emceelovin commented 4 years ago

@sungam3r I'm going to have to check that out. I must have overlooked that.