Let legacy ws clients know about invalid data frames

DoctorJohn commented 2 months ago

Starting with this PR, we will let legacy WS clients know when they send data frames (e.g., binary instead of text data frames) that are not supported by the protocol instead of simply ignoring them.

Description

While working on unifying all WebSocket implementations, I noticed that our implementation of the legacy WS protocol currently silently ignores unsupported data frames (i.e., binary data frames). So, I rechecked the protocol specs. They say only "stringified" JSON messages are allowed. That's the same formulation used in the newer WS protocol, where it is used to indicate that only text data frames are supported (related discussion: https://github.com/enisdenjo/graphql-ws/issues/409).

Starting with this PR, we report the use of unsupported data frames to clients (before, we ignored them). This should not cause any disruption to clients since they usually operate in text or binary mode. If they operate in text mode, everything will keep working. If they operate in binary mode, they never worked but will receive an error message now.

Close code 1002 was chosen because that code is also used in the reference implementation of the legacy protocol to signal protocol violations. The generally vague specs don't stipulate any more specific code for this scenario.

While at it, I unified the error message across both protocols and all integrations. (With the upcoming unification PR this would have been the case anyways).

Types of Changes

[ ] Core
[ ] Bugfix
[ ] New feature
[x] Enhancement/optimization
[ ] Documentation

Checklist

[ ] My code follows the code style of this project.
[ ] My change requires a change to the documentation.
[ ] I have updated the documentation accordingly.
[ ] I have read the CONTRIBUTING document.
[x] I have added tests to cover my changes.
[x] I have tested the changes and verified that they work and don't break anything (as well as I can manage).

Summary by Sourcery

Enhance the legacy WebSocket protocol implementation to notify clients when unsupported binary data frames are sent, closing the connection with a specific error code and reason. Update tests to reflect this change and add documentation to inform users of the new behavior.

Enhancements:

Notify legacy WebSocket clients of unsupported binary data frames by closing the connection with code 1002 and a specific reason.

Documentation:

Add a release note indicating that legacy graphql-ws clients will now receive an error for binary data frames instead of them being ignored.

Tests:

Update tests to verify that non-text WebSocket messages result in a connection closure with the appropriate close code and reason.

sourcery-ai[bot] commented 2 months ago

Reviewer's Guide by Sourcery

This pull request enhances the WebSocket implementation for legacy graphql-ws clients by adding error reporting for unsupported data frames. Previously, binary data frames were silently ignored, but now the server will close the connection with an error message when receiving such frames. This change improves protocol compliance and provides better feedback to clients.

File-Level Changes

Change	Details	Files
Implement error handling for non-text WebSocket messages	Add close code 1002 for protocol violations Set error message 'WebSocket message type must be text' Replace silent ignoring of binary messages with connection closure Update test cases to reflect new behavior	`strawberry/asgi/handlers/graphql_ws_handler.py` `strawberry/litestar/handlers/graphql_ws_handler.py` `strawberry/aiohttp/handlers/graphql_ws_handler.py` `strawberry/channels/handlers/graphql_ws_handler.py` `tests/websockets/test_graphql_ws.py` `tests/websockets/test_graphql_transport_ws.py`
Unify error handling across different WebSocket implementations	Standardize error message and close code across handlers Update channels integration to use the new error handling approach	`strawberry/channels/handlers/ws_handler.py` `strawberry/channels/handlers/graphql_ws_handler.py`
Add release notes	Create RELEASE.md file with patch release information Explain the change in behavior for legacy graphql-ws subprotocol	`RELEASE.md`

Tips

- Trigger a new Sourcery review by commenting `@sourcery-ai review` on the pull request. - Continue your discussion with Sourcery by replying directly to review comments. - You can change your review settings at any time by accessing your [dashboard](https://app.sourcery.ai): - Enable or disable the Sourcery-generated pull request summary or reviewer's guide; - Change the review language; - You can always [contact us](mailto:support@sourcery.ai) if you have any questions or feedback.

botberry commented 2 months ago

Thanks for adding the RELEASE.md file!

Here's a preview of the changelog:

Starting with this release, clients using the legacy graphql-ws subprotocol will receive an error when they try to send binary data frames. Before, binary data frames were silently ignored.

While vaguely defined in the protocol, the legacy graphql-ws subprotocol is generally understood to only support text data frames.

Here's the tweet text:

🆕 Release (next) is out! Thanks to @NucleonJohn for the PR 👏

Get it here 👉 https://strawberry.rocks/release/(next)

botberry commented 2 months ago

Thanks for adding the RELEASE.md file!

Here's a preview of the changelog:

Starting with this release, clients using the legacy graphql-ws subprotocol will receive an error when they try to send binary data frames. Before, binary data frames were silently ignored.

While vagugely defined in the protocol, the legacy graphql-ws subprotocol is generally understood to only support text data frames.

Here's the tweet text:

🆕 Release (next) is out! Thanks to @NucleonJohn for the PR 👏

Get it here 👉 https://strawberry.rocks/release/(next)

codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 94.78%. Comparing base (2941146) to head (6c08040). Report is 1 commits behind head on main.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #3633 +/- ## ========================================== - Coverage 96.76% 94.78% -1.98% ========================================== Files 522 518 -4 Lines 33796 32647 -1149 Branches 5635 3772 -1863 ========================================== - Hits 32703 30945 -1758 - Misses 862 1408 +546 - Partials 231 294 +63 ```

codspeed-hq[bot] commented 2 months ago

CodSpeed Performance Report

Merging #3633 will not alter performance

_{Comparing DoctorJohn:inform-clients-about-invalid-data-frames (6c08040) with main (2941146)}

Summary

✅ 15 untouched benchmarks

strawberry-graphql / strawberry