search

graphql-dotnet / graphql-client

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

Netwtonsoft `MissingMemberHandling.Error` is incompatible with receiving web socket messages #660

Open JR-Morgan opened 1 month ago

JR-Morgan commented 1 month ago

My goal is to be able to utilise the MissingMemberHandling.Error option of the JsonSerializerSettings, instead of the default MissingMemberHandling.Ignore.

Why? because its a very useful way to ensure that the classes I'm crafting as deserialization targets for my queries actually match the queries being sent. It makes it impossible to accidently query for something that will be ignored during serialization, as this will throw a JsonSerializationException

This works perfectly for regular queries and mutations. But not for subscriptions or UseWebSocketForQueriesAndMutations = true

Instead, I get this exception thrown deep from ReceiveWebsocketMessagesAsync

Newtonsoft.Json.JsonSerializationException: Could not find member 'payload' on object of type 'WebsocketMessageWrapper'. Path 'payload', line 1, position 65.
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.PopulateObject(Object newObject, JsonReader reader, JsonObjectContract contract, JsonProperty member, String id)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.CreateObject(JsonReader reader, Type objectType, JsonContract contract, JsonProperty member, JsonContainerContract containerContract, JsonProperty containerMember, Object existingValue)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.CreateValueInternal(JsonReader reader, Type objectType, JsonContract contract, JsonProperty member, JsonContainerContract containerContract, JsonProperty containerMember, Object existingValue)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.Deserialize(JsonReader reader, Type objectType, Boolean checkAdditionalContent)
   at Newtonsoft.Json.JsonSerializer.DeserializeInternal(JsonReader reader, Type objectType)
   at Newtonsoft.Json.JsonSerializer.Deserialize(JsonReader reader, Type objectType)
   at Newtonsoft.Json.JsonSerializer.Deserialize[T](JsonReader reader)
   at GraphQL.Client.Serializer.Newtonsoft.NewtonsoftJsonSerializer.DeserializeFromUtf8Stream[T](Stream stream) in /_/src/GraphQL.Client.Serializer.Newtonsoft/NewtonsoftJsonSerializer.cs:line 54
   at GraphQL.Client.Serializer.Newtonsoft.NewtonsoftJsonSerializer.DeserializeToWebsocketResponseWrapperAsync(Stream stream) in /_/src/GraphQL.Client.Serializer.Newtonsoft/NewtonsoftJsonSerializer.cs:line 41
   at GraphQL.Client.Http.Websocket.GraphQLHttpWebSocket.ReceiveWebsocketMessagesAsync() in /_/src/GraphQL.Client/Websocket/GraphQLHttpWebSocket.cs:line 524

I've captured the offending response from the server. And it does indeed have a payload property, (the value of which is all from my GraphQL api)

  "{\"type\":\"data\",\"id\":\"80fdadc12c0b44bc84115ce7231cb096\",\"payload\":{\"data\":{\"data\":{\"comment\":{\"id\":\"1408af422d\"},\"id\":\"1408af422d\",\"type\":\"CREATED\"}}}}"

But poking around a little, this seems like an indented outcome seeing as WebsocketMessageWrapper does not need to store the payload

I was wondering if this was intentional, as it's preventing me from using this serializer setting for my entire SDK.

rose-a commented 1 month ago

Hi, the problem is that the payload field is not always there in the websocket protocol. What's in there can only be decided after the first deserialization and evaluating the type field.

The easiest way to solve your problem you would be to implement your own NewtonsoftJsonSerializer, which uses a different instance of json serializer settings for DeserializeToWebsocketResponseWrapperAsync, which is where your exception is thrown and which is the "partial" serialization step needed to get the type field from the message.

I'd be glad to incorporate that "separation" of serializer settings into the library if you submit that as a PR.

JR-Morgan commented 1 month ago

Amazing, thanks. I'm already using a fork of the NewtonsoftJsonSerializer so tweaking it is quite easy for me.

https://github.com/specklesystems/speckle-sharp-sdk/blob/b986a1bc6f737e1a83b861e0c9c1ffab361ec29a/src/Speckle.Sdk/Api/GraphQL/Serializer/NewtonsoftJsonSerializer.cs#L49

I've just tested with making the DeserializeToWebsocketResponseWrapperAsync always use the DefaultJsonSerializerSettings , as this was simplest, and I don't see any valid reason users would ever want to customise this JsonSerializerSetting since its only used for deserializing WebsocketMessageWrappers.

This is now working great for me 😁

Would you like this as a PR?

rose-a commented 1 month ago

Yea please do a PR. I think this should have been there from the start...