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

Add GraphQL subscriptions support via WebSocket #14

Closed
kinow opened this issue Apr 4, 2019 · 6 comments · Fixed by #82
Closed

Add GraphQL subscriptions support via WebSocket #14

kinow opened this issue Apr 4, 2019 · 6 comments · Fixed by #82
Assignees
Milestone

Comments

@kinow
Copy link
Member

kinow commented Apr 4, 2019

See #11 for previous discussion. The UI Server is a Tornado project, that inherits classes from JupyterHub Tornado project (for things like authentication).

In #11 and #13 it's getting an extra layer with Graphene, an open source library for Python that implements GraphQL.

However, GraphQL subscriptions require the base layer of Graphene for GraphQL, plus an extra library to handle the query subscription (which is basically a mechanism for sending to clients the output of a query whenever if changed), the normal way being via WebSockets.

WebSockets are supported by Tornado out of the box. What we are missing, is actually the extra layer of integration with Tornado. That is done via the Graphele graphql-ws project, which has one Pull Request to add support to tornado still pending.

Once that pull request is merged, we can add that library in our setup.py, and fix this issue. An alternative in case we need that before the pull request is merged, is simply fork it and use our own until it is merged.

@kinow
Copy link
Member Author

kinow commented Jul 16, 2019

The PR linked here was failing in their Travis setup, due to use of pathlib (Python 3 stdlib). Pinged them pointing the issue, and also to bring it back to the devs backlog 🤞

@kinow
Copy link
Member Author

kinow commented Sep 4, 2019

Tried setting up a small project to test WebSockets + Vue, using the pending pull request.

Alas found a few issues in graphene-tornado, that I think block this issue.

Though I am hoping to be wrong, and that we just need to use the API in a different way to work around the issues I found 🤞

@kinow
Copy link
Member Author

kinow commented Sep 5, 2019

When implementing the Tornado handler for WebSockets, it's important to remember to allow different origins. See their docs for reference. Here's how to disable globally (bad probably?)

def check_origin(self, origin):
    return True

Ideally we would either disable globally but add a big # TODO: review before full release, for security, or use something similar to their other example:

def check_origin(self, origin):
    parsed_origin = urllib.parse.urlparse(origin)
    return parsed_origin.netloc.endswith(".mydomain.com")

Though it's not clear to me what information we would have at that point to validate the other origin.

@kinow
Copy link
Member Author

kinow commented Sep 5, 2019

Another note (thanks to @dwsutherland for reminding me), should a message like the following is displayed in logs:

image

This is caused due to a missing property in Tornado handler executor. We need to include the allow_subscriptions=True and pass that to the underlying executor (see linked tickets above for more)

@kinow kinow added this to the 0.1 milestone Sep 10, 2019
@kinow kinow modified the milestones: 0.1, 0.2 Sep 18, 2019
@kinow
Copy link
Member Author

kinow commented Sep 18, 2019

Moving to 0.2 as this task requires further work on both Cylc UI Server & Cylc UI, and will take time - but hopefully can be delivered in 0.2

This was referenced Sep 26, 2019
@kinow kinow self-assigned this Oct 1, 2019
@kinow
Copy link
Member Author

kinow commented Oct 1, 2019

Doing the Python part this week, aiming at starting this task next Monday 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant