Let legacy ws clients know about invalid data frames

[DoctorJohn]

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: enisdenjo/graphql-ws#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
• 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.
• I have added tests to cover my changes.
• 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]

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:
  • Enable or disable the Sourcery-generated pull request summary or reviewer's guide;
  • Change the review language;
• You can always contact us if you have any questions or feedback.

[sourcery-ai]

Hey @DoctorJohn - I've reviewed your changes and they look great!

Here's what I looked at during the review

• 🟡 General issues: 1 issue found
• 🟢 Security: all looks good
• 🟡 Testing: 2 issues found
• 🟢 Complexity: all looks good
• 🟡 Documentation: 1 issue found

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.}

[botberry]

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]

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]

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files

@@            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]

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

[erikwrede]

shouldn't this say subscriptions-transport-ws? graphql-ws is the newer one, and then graphql-over-ws is the current in-progress spec to standardize GraphQL on this protocol

[DoctorJohn]

That's a classic Erik: The new subprotocol is called graphql-transport-ws and is living in a repository called graphql-ws, while the old subprotocol is called graphql-ws dying in a repository called subscriptions-transport-ws. Throughout our code and docs we refer to the subprotocols name, instead of the repository names.

[DoctorJohn]

For reference, here's the relevant line in the newer protocols spec: https://github.com/enisdenjo/graphql-ws/blob/c030ed1d5f7e8a552dffbfd46712caf7dfe91a54/PROTOCOL.md?plain=1#L10

The legacy protocol's spec is a mess, but the subprotocol's name (graphql-ws) can be found in the reference implementation and various clients supporting it.

[erikwrede]

The title of the document is literally graphql over ws and that is the new nomenclature for all the other transport protocols :D lets hope this becomes more consistent in the future

[DoctorJohn]

Same. This caused way too much confusion over the years 🫠

[erikwrede]

Gonna submit an issue let's see if Denis agrees 😂