zino-hofmann / graphql-flutter

A GraphQL client for Flutter, bringing all the features from a modern GraphQL client to one easy to use package.
https://zino-hofmann.github.io/graphql-flutter
MIT License
3.25k stars 620 forks source link

Allow list of errors as payloads for graphql-transport-ws subprotocol's "error" message type #1242

Closed juancastillo0 closed 2 years ago

juancastillo0 commented 2 years ago

Fixes https://github.com/zino-hofmann/graphql-flutter/issues/1241.

This allows the payload property in the response to be a List in the GraphQLSocketMessage.parse function. Specifically for the SubscriptionError class since the payload should be a List of errors for messages of type error following the graphql-transport-ws subprotocol specification.

It was tested with a leto server here.

Thanks!

juancastillo0 commented 2 years ago

Hi, thanks!

I believe the commit message follows the guide and I removed the merge commit.

Sorry for the delay and if there is something more I can do, please, let me know!

vincenzopalazzo commented 2 years ago

Thanks! the failure in the CI looks unrelated for now, could you please rebase on main because I fixed the failure in https://github.com/zino-hofmann/graphql-flutter/pull/1243

ristiisa commented 2 years ago

Is there some way to understand if the code works as expected? or maybe you can write a couple of lines of the test? and why this cast is required only for some messages like SubscribeOperation and not for InitOperation?

This PR move the cast couple of line after, and I do not see how this can improve the code or solve the casting error, but I'm sure that I'm missing something, can you point out what I'm missing?

I guess the idea is that the payload is an Array only when there is an error and the cast to map is moved to later...

juancastillo0 commented 2 years ago

The casting is performed afterwards since the type of payload is Object, it is not a Map<String, dynamic> because it could be a List as is the case for an error message type.

For error, connectionInit and connectionError, the payload type is dynamic, no casting is required (it will be a List for errors in the graphql-transport-ws subprotocol). However, for subscribe, start, ping and pong a casting is performed since the payload for those messages should be a Map<String, dynamic>. Basically, I added a casting for payload where it was required. Before, the casting was performed at the beginning which was causing the exception, since the List sent from the server was being cast as a Map<String, dynamic>.

Yes, I was planning on making a test, but I did not consider it necessary since it was a bug fix and it was a small change, but I agree we could try to make a test to make sure a List of errors can be parsed. Do you think we should test only the function or connect it completely with the mock GraphQL server?

vincenzopalazzo commented 2 years ago

For error, connectionInit and connectionError, the payload type is dynamic, no casting is required (it will be a List for errors in the graphql-transport-ws subprotocol). However, for subscribe, start, ping and pong a casting is performed since the payload for those messages should be a Map<String, dynamic>. Basically, I added a casting for payload where it was required. Before, the casting was performed at the beginning which was causing the exception, since the List sent from the server was being cast as a Map<String, dynamic>.

I see now thanks! I was missing the part of the spec!

Do you think we should test only the function or connect it completely with the mock GraphQL server?

I think just a function is enough, the mock server that we currently have is too monkey to waste time in write test with it :)

codecov[bot] commented 2 years ago

Codecov Report

Merging #1242 (423adb3) into main (6a5caf9) will increase coverage by 0.47%. The diff coverage is 60.00%.

@@            Coverage Diff             @@
##             main    #1242      +/-   ##
==========================================
+ Coverage   59.29%   59.77%   +0.47%     
==========================================
  Files          41       41              
  Lines        1683     1683              
==========================================
+ Hits          998     1006       +8     
+ Misses        685      677       -8     
Impacted Files Coverage Δ
...b/src/links/websocket_link/websocket_messages.dart 57.39% <60.00%> (+6.95%) :arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.