Migrate graphql-transport-ws types to TypedDicts #3701
Conversation
Reviewer's Guide by SourceryThis PR migrates the graphql-transport-ws protocol types from dataclasses to TypedDicts. The implementation focuses on using TypedDict's type system to more precisely model protocol message types, particularly distinguishing between null and undefined values. The changes eliminate the need for custom conversion methods and UNSET handling, while maintaining the same protocol functionality. Class diagram for graphql-transport-ws TypedDictsclassDiagram
class ConnectionInitMessage {
type: "connection_init"
payload: NotRequired<Union<Dict[str, object], None>>
}
class ConnectionAckMessage {
type: "connection_ack"
payload: NotRequired<Union<Dict[str, object], None>>
}
class PingMessage {
type: "ping"
payload: NotRequired<Union<Dict[str, object], None>>
}
class PongMessage {
type: "pong"
payload: NotRequired<Union<Dict[str, object], None>>
}
class SubscribeMessagePayload {
query: str
operationName: NotRequired<Union[str, None]>
variables: NotRequired<Union<Dict[str, object], None>>
extensions: NotRequired<Union<Dict[str, object], None>>
}
class SubscribeMessage {
id: str
type: "subscribe"
payload: SubscribeMessagePayload
}
class NextMessagePayload {
errors: NotRequired<List[GraphQLFormattedError]>
data: NotRequired<Union<Dict[str, object], None>>
extensions: NotRequired<Dict[str, object]>
}
class NextMessage {
id: str
type: "next"
payload: NextMessagePayload
}
class ErrorMessage {
id: str
type: "error"
payload: List[GraphQLFormattedError]
}
class CompleteMessage {
id: str
type: "complete"
}
class Message {
<<Union>>
ConnectionInitMessage
ConnectionAckMessage
PingMessage
PongMessage
SubscribeMessage
NextMessage
ErrorMessage
CompleteMessage
}
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @DoctorJohn - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Thanks for adding the Here's a preview of the changelog: In this release, we migrated the Here's the tweet text: |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3701 +/- ##
==========================================
- Coverage 97.01% 96.70% -0.31%
==========================================
Files 502 502
Lines 33568 33608 +40
Branches 5634 5624 -10
==========================================
- Hits 32565 32500 -65
- Misses 796 885 +89
- Partials 207 223 +16 |
DoctorJohn
force-pushed
the
migrate-graphql-transport-ws-to-typeddicts
branch
from
dea76f2
to
64ccfa9
Compare
CodSpeed Performance ReportMerging #3701 will not alter performanceComparing Summary
|
|
|
||
| if message_type == ConnectionInitMessage.type: | ||
| await self.handle_connection_init(ConnectionInitMessage(**message)) | ||
| elif message["type"] == "ping": |
There was a problem hiding this comment.
Why are we switching from constants to strings here? I get that these strings are obvious, but my intuition tells me a constant suits this better
There was a problem hiding this comment.
I want the new TypedDicts to be the single source of truth for the shape of graphql-transport-ws messages. However, unlike with dataclassses, it's not possible to store a constant value in a TypedDict. The only option is to use typing.Literal and let the type checker enforce that message["type"] must always match one of the literal message types. (which is what we do here).
Just to clarify: the right side of the == has autocompletion and type checkers riot if a value is put there which does not match any of the message types.
There was a problem hiding this comment.
I think with types, plain strings are better 😊
| @@ -198,11 +180,11 @@ async def handle_connection_init(self, message: ConnectionInitMessage) -> None: | |||
| return | |||
|
|
|||
| self.connection_init_received = True | |||
| await self.send_message(ConnectionAckMessage()) | |||
| await self.websocket.send_json(ConnectionAckMessage({"type": "connection_ack"})) | |||
There was a problem hiding this comment.
As much as I like the improvement stemming from the dicts above, I really dislike this redundancy of types. What about adding a new create classmethod on the TypedDict class that handles filling this value? Not very pythonic, but at least you don't need to pass the message types again
There was a problem hiding this comment.
Similar story to the previous comment: At runtime TypedDicts are just regular dicts. When subclassing TypedDict we can't add any custom methods (or constants) to it.
While this new style looks more verbose, it's also more explicit since it's no longer hiding away any message fields. I honestly like it, especially since it makes pyright and mypy super happy.
Here's something we could do with the hightlighted code:
self.websocket.send_json takes any dict as argument, that's why we wrap our message dict in ConnectionAckMessage so that the type checker ensures it has a valid shape.
Instead we could add and call a (new) send_message(self, message: Message) method, which expects a valid Message as argument. In the example above, we would no longer need to wrap our typed dict in ConnectionAckMessage, because the type checker would ensure that the message's shape is valid based on it's type field.
So instead of:
await self.websocket.send_json(COnnectionAckMessage({"type": "connection_ack"})
we would write:
await self.send_message({"type": "connection_ack"})
and type checker would still provide autocompletion and enforce that messages have a valid shape.
There was a problem hiding this comment.
(Just added two commits doing exactly that)
|
|
||
| if message_type == ConnectionInitMessage.type: | ||
| await self.handle_connection_init(ConnectionInitMessage(**message)) | ||
| elif message["type"] == "ping": |
There was a problem hiding this comment.
I think with types, plain strings are better 😊
Description
I recently migrated the legacy ws protocols types to typed dicts (#3689), in this PR the modern ws protocols types are migrated to typed dicts. This, again, comes with some nice benefits such as:
nullvsundefinedvalues.to_dict()methods on dataclasses to avoid performance penaltiesUNSETconversionsAny)Types of Changes
Summary by Sourcery
Enhancements: