-
Notifications
You must be signed in to change notification settings - Fork 70
Fix bugs with transport-ws-protocol #111
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
Fix bugs with transport-ws-protocol #111
Conversation
I am guessing this branch was only used with GraphQLWSHandler, and not with GraphQLTransportWSHandler. That's probably why there were a bunch of bugs. This PR fixes those.
return await self._get_context(request=self._ws) | ||
return await self._get_context(self._ws) | ||
|
||
async def get_root_value(self) -> Any: | ||
return await self._get_root_value(request=self._ws) | ||
return await self._get_root_value(self._ws) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed this because channels/graphql_ws.py doesn't specify the argument name either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Side note: graphql_ws.py
and graphql_transport_ws.py
have alot of duplicate code that could be merged. I will not do that in this PR though.
await self._ws.accept( | ||
subprotocol=BaseGraphQLTransportWSHandler.GRAPHQL_TRANSPORT_WS_PROTOCOL | ||
) | ||
await self._ws.accept(subprotocol=BaseGraphQLTransportWSHandler.PROTOCOL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wrong variable name was being used :/
async def send_message(self, data: Message) -> None: | ||
data = data.asdict() | ||
assert ( | ||
data.get("type") is not None | ||
), "expected dict with `type` field. Got {} instead".format(data) | ||
await self.send_json(data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use dataclasses instead of TypedDicts. TypeDict
is just a dict at runtime, and is only used for typehinting. That means data.type
would not have worked.
sidenote: we should do the same for graphql_ws.py
too.
@@ -5,6 +5,8 @@ | |||
except ImportError: | |||
from typing_extensions import TypedDict | |||
|
|||
from dataclasses import dataclass, asdict | |||
|
|||
from .contstants import ( | |||
GQL_CONNECTION_INIT, | |||
GQL_CONNECTION_ACK, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These constants should probably be an enum, but fixing that shouldn't be part of this PR
I am guessing the
v3
branch was only used withGraphQLWSHandler
, and not withGraphQLTransportWSHandler
. That's probably why there were a bunch of bugs. This PR fixes the errors I got when trying to use subscriptions with thegraphql-transport-ws
protocol.The first error I got was
I implemented this function and the error went away.
Second error was with message types - they were
TypedDict
s, but typeddicts are essentially dicts.This means additional fields specified are not properly used. I fixed it by converting them to
dataclasses
and adding ato_dict
field to it.