Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GraphQL Subscriptions Support #23

Merged
merged 6 commits into from
Jun 8, 2020
Merged

GraphQL Subscriptions Support #23

merged 6 commits into from
Jun 8, 2020

Conversation

DaleSeo
Copy link
Contributor

@DaleSeo DaleSeo commented Apr 11, 2020

What kind of change does this PR introduce?

This PR adds support for GraphQL subscriptions to the client. (fix #4)

What is the current behavior?

The client only supports GraphQL queries and mutations.

What is the new behavior?

The client also supports GraphQL subscriptions.

This PR implements the subscribe method of the GraphqlClient class. The websockets dependency is added to the project because GraphQL subscriptions are executed over WebSocket unlike GraphQL queries and mutations.

It wasn't easy to find public APIs that support GraphQL subscriptions. So, I wrote a very simple API myself for understanding how GraphQL subscriptions work and also for performing some manual testing.

image

The above screenshot is what I captured from the network tab of Chrome Developer Tools after initiating the messageAdded subscription. Once the endpoint receives a request in the form of {"type": "start", "id": "1", "payload": {"query": ..., "variables": ...}}, it sends back with a series of responses like {"type": "start", "id": "1", "payload": {"data": ...}}.

My implementation in this PR is simply based on these behaviors between the client and the server that I observed on the Apollo Playground tool. There must be huge room for improvement but I hope this serves as the first step in enabling the client to support all GraphQL operations.

Here're the manual testing results.

import asyncio
from python_graphql_client import GraphqlClient

client = GraphqlClient(endpoint="wss://3wqzw.sse.codesandbox.io/graphql")

query = """
    subscription onMessageAdded {
        messageAdded
    }
"""

asyncio.run(client.subscribe(query=query, handle=print))
$ python tests/dale.py                                                    
{'data': {'messageAdded': 'Vero autem voluptatem architecto.'}}
{'data': {'messageAdded': 'Est quo sunt est repellendus perspiciatis qui culpa reprehenderit.'}}
{'data': {'messageAdded': 'Inventore nemo quos illo magni voluptates autem quia.'}}
{'data': {'messageAdded': 'Eos omnis minima.'}}
{'data': {'messageAdded': 'Perspiciatis id vitae iure provident omnis assumenda sapiente eum.'}}
{'data': {'messageAdded': 'Dolores autem est.'}}
{'data': {'messageAdded': 'Accusantium temporibus impedit soluta beatae occaecati unde magnam sit.'}}

Does this PR introduce a breaking change?

No. This doesn't touch any of the existing functionalities.

Other information

@DaleSeo DaleSeo added the enhancement New feature or request label Apr 11, 2020
@DaleSeo DaleSeo self-assigned this Apr 11, 2020
@steve148
Copy link
Contributor

Sorry for being slow with getting a review on this PR, but here's my two cents.

I think the simple implementation you did here is really nice / easy to understand. I'm playing with this locally now to see how the project gets affected by the different possible message types and whether or not how this client handles the message makes sense. For example, if the server sends the keep alive type ka does our client wait until it receives a proper message with a data payload?

Otherwise once we have a clearer understanding of what the endge cases are for this feature, then I think some more tests to document that would be helpful.

@steve148
Copy link
Contributor

I'm going through the Graphql specification docs right now as I'm also unsure whether we should be using the python graphql websocket protocol https://github.com/graphql-python/graphql-ws or the apollo graphql websocket protocol https://github.com/apollographql/subscriptions-transport-ws.

Copy link
Contributor

@steve148 steve148 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When testing this locally against the test server you had setup the suscribe logic seemed to work fine, but when testing it against another graphql server with a demo subscriotion resolver it seems to fail.

The documentation on how to start a graphql subscriptions isn't really clear, but from what I tell we need to implement a handshake logic in the subscribe function to make sure we're following the apollo protocol.

The following resources were helpful for understanding the subscription stuff more:

Also if you want to test against the demo server I tried locally, I cloned this repository and ran the server https://github.com/nicolasdao/apollo-subscription-demo.

@DaleSeo
Copy link
Contributor Author

DaleSeo commented May 9, 2020

@steve148 Thanks for your guidance and sorry for the delay in addressing your feedback. I read through the material you shared and had a better understanding of the protocol.

I've added the handshake logic that you mentioned. I've tested it against the demo server you tried and it seems to work properly. It also still works with the test server that I had set up as it did without the handshake logic.

May-09-2020 17-09-25

Below is the code snippet that I used for manual testing if you want to try on your own.

import asyncio
from python_graphql_client import GraphqlClient

client = GraphqlClient(endpoint="ws://localhost:4000/subscriptions")

query = """
    subscription {
        messageAdded(channelId: 1) {
            id
            text
        }
    }
"""

asyncio.run(client.subscribe(query=query, handle=print))

Copy link
Contributor

@steve148 steve148 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now my turn to apologize for being slow on this PR (a month too slow). I've been having computer issues lately forcing me to work on a non-ideal machine.

Thanks for making the changes, tested it and it seems good 👍 It is super simple which also makes it really easy to understand. One note is that it currently does not have a graceful way to handle the graphql server closing a websocket / subscription. Assuming all things work well then this is ok, but in the future we may want to do a better job of alerting the process with a better message for the websocket shutting down.

@DaleSeo
Copy link
Contributor Author

DaleSeo commented Jun 8, 2020

One note is that it currently does not have a graceful way to handle the graphql server closing a websocket / subscription. Assuming all things work well then this is ok, but in the future we may want to do a better job of alerting the process with a better message for the websocket shutting down.

Totally agreed! We can work on it together down the road. 😎

@DaleSeo DaleSeo merged commit 391fc9b into prodigyeducation:master Jun 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for graphql subscriptions
2 participants