-
-
Notifications
You must be signed in to change notification settings - Fork 536
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
Let WS and HTTP use the same JSON encoder #3708
Let WS and HTTP use the same JSON encoder #3708
Conversation
Reviewer's Guide by SourceryThis PR unifies JSON encoding across HTTP and WebSocket responses by making the Sequence diagram for WebSocket message encodingsequenceDiagram
participant Client
participant WebSocketAdapter
participant AsyncBaseHTTPView
participant WebSocket
Client->>WebSocketAdapter: Send message
WebSocketAdapter->>AsyncBaseHTTPView: encode_json(message)
AsyncBaseHTTPView-->>WebSocketAdapter: Encoded JSON
WebSocketAdapter->>WebSocket: send_text(encoded JSON)
WebSocket-->>Client: Acknowledge receipt
Updated class diagram for AsyncWebSocketAdapterclassDiagram
class AsyncWebSocketAdapter {
+AsyncBaseHTTPView view
+iter_json(ignore_parsing_errors: bool)
+send_json(message: Mapping[str, object])
+close(code: int, reason: str)
}
class AsyncBaseHTTPView {
+encode_json(data: object) : str
}
AsyncWebSocketAdapter --> AsyncBaseHTTPView: uses
note for AsyncWebSocketAdapter "Now uses view's encode_json method for JSON encoding"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @DoctorJohn - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 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.
Thanks for adding the Here's a preview of the changelog: Starting with this release, the same JSON encoder is used to encode HTTP This enables developers to override the Here's the tweet text:
|
af00c2d
to
d8917a5
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3708 +/- ##
==========================================
+ Coverage 96.71% 97.01% +0.30%
==========================================
Files 500 500
Lines 33436 33454 +18
Branches 5590 5591 +1
==========================================
+ Hits 32336 32456 +120
+ Misses 883 794 -89
+ Partials 217 204 -13
|
CodSpeed Performance ReportMerging #3708 will not alter performanceComparing Summary
|
your method can handle the same inputs as the built-in `json.dumps` method: | ||
|
||
```python | ||
def encode_json(self, data: object) -> str: ... |
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.
could we type this better? is it worth it?
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.
json.loads
expects Any
so I went with object
. Technically it's anything that can be serialized to JSON (list
, dict
, int
, float
, str
, etc.). But the json
library doesn't provide a type for it which we could reuse. We would have to create our own, pretty sure it's not worth it for this method.
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.
@DoctorJohn mmh, is it because the ws integration can send pretty much anything, right?
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.
it because the ws integration can send pretty much anything
Not necessarily, the WS messages are all quite well defined. If we wanted to limited the input to the encode_json
method we could go with Union[GraphQLHTTPResponse, Message, OperationMessage]
.
However, this signature would signal to users that they should implement a method that can specifically/only encode these messages. If someone does that, they would have to update their encode_json
method every time we add a new protocol or any of the specs changes.
IMHO we should ask the user to write encode_json
methods that can encode any JSON payload and not just a subset, so that they don't worry about specific payloads and so that we can use that method for any JSON that needs to be encoded from within views.
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.
ok, sounds good to me 😊
Description
It was already possible and documented to override
encode_json
on views to customize the JSON encoder used for responses sent via HTTP. The sameencode_json
method is now also used to encode WebSocket messages.This was proposed a few weeks ago by @bellini666 and it's super handy if someone wants to use
orjson
or otherwise customize JSON responses and messages.Note that this change is kinda breaking since the
encode_json
methode previously only expectedGraphQLHTTPResponse
dicts, whereas now it must be able to handle any kind of json data.In reality anyone overriding this method can probably already handle any json data.
(Will do the same for decoding in a separate PR).
Types of Changes
Summary by Sourcery
Unify the JSON encoding process for HTTP and WebSocket by using the same encode_json method, allowing for consistent customization across both protocols. Update documentation and add tests to support this change.
New Features:
Documentation:
Tests: