fix: make payload:null handling in connection_init consistent between protocols #3744
Conversation
Reviewer's Guide by SourceryThis pull request fixes an inconsistency in how Sequence diagram showing updated connection_init message handlingsequenceDiagram
participant Client
participant GraphQL_WS as GraphQL WS Protocol
participant GraphQL_Transport_WS as GraphQL Transport WS Protocol
Note over Client,GraphQL_Transport_WS: Both protocols now handle payload:null consistently
Client->>GraphQL_WS: connection_init (payload:null)
GraphQL_WS-->>Client: Convert null to {} and continue
Client->>GraphQL_Transport_WS: connection_init (payload:null)
GraphQL_Transport_WS-->>Client: Convert null to {} and continue
State diagram for connection_init payload handlingstateDiagram-v2
[*] --> CheckPayload: Receive connection_init
CheckPayload --> ConvertNull: payload is null
CheckPayload --> ValidateType: payload exists
ConvertNull --> ValidateType: convert to {}
ValidateType --> Success: is dictionary
ValidateType --> Error: not dictionary
Success --> [*]: Continue connection
Error --> [*]: Close connection
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 @Object905 - 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: all looks good
- 🟢 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.
|
Hi, thanks for contributing to Strawberry 🍓! We noticed that this PR is missing a So as soon as this PR is merged, a release will be made 🚀. Here's an example of Release type: patch
Description of the changes, ideally with some examples, if adding a new feature.Release type can be one of patch, minor or major. We use semver, so make sure to pick the appropriate type. If in doubt feel free to ask :) Here's the tweet text: |
| @@ -97,7 +97,9 @@ async def handle_message( | |||
|
|
|||
| async def handle_connection_init(self, message: ConnectionInitMessage) -> None: | |||
| payload = message.get("payload") | |||
| if payload is not None and not isinstance(payload, dict): | |||
| if payload is None: | |||
There was a problem hiding this comment.
For graphql_ws before this PR connection_params could be null, if payload is null.
So this may be a slightly breaking change.
| @@ -167,7 +167,8 @@ async def handle_connection_init(self, message: ConnectionInitMessage) -> None: | |||
| self.connection_init_timeout_task.cancel() | |||
|
|
|||
| payload = message.get("payload", {}) | |||
|
|
|||
| if payload is None: | |||
There was a problem hiding this comment.
Not breaking for graphql_transport_ws, because it was required to be dict.
It's possible to make other way around and allow null in graphql_transport_ws, like in graphql_ws.
But since it's "recommended" protocol I decided to change graphql_ws and not the other way around.
|
Hi @Object905, I noticed this recently myself and planned to do two things:
Your PR essentially addresses (1.), that's great, but it might be worth it to do both things in the same PR so that we only have one breaking release. What do you think? You could also just do (1.) and make sure both handlers accept the payloads defined by their protocols (I recommend checking the Either way, we would need tests for these changes. |
When client specifies
payload: nullin connection_init message graphql_transport_ws erored withInvalid connection init payload, while ingraphql_wsthis case was handled.Noticed this with latest apollo-router as a client.
Summary by Sourcery
Bug Fixes:
payload: nullin the connection_init message, while graphql_ws handled this case correctly.