Feature: implement steaming api responses#129
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an explicit Content-Encoding: identity header to the NDJSON streaming route and validates it in the existing streaming-route test.
Changes:
- Set
Content-Encoding: identityonStreamingResponsereturned bystreaming_json_route. - Update the streaming route test to assert the new response header.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
core/api/streaming_json_route.py |
Adds an explicit Content-Encoding header to the streaming NDJSON response. |
tests/api/test_streaming_json_route.py |
Extends the valid-body test to assert content-encoding is identity. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return StreamingResponse( | ||
| content=wrappedGenerator, | ||
| media_type='application/x-ndjson', | ||
| headers={'Content-Encoding': 'identity'}, |
There was a problem hiding this comment.
Setting Content-Encoding: identity is redundant (no encoding is the default when the header is absent) and can be misinterpreted by some HTTP clients/middleware as an actual encoding to decode. If the intent is to prevent response compression for streaming, prefer disabling compression at the middleware/server layer, or omit Content-Encoding and use a more appropriate directive (e.g., Cache-Control: no-transform) if you need to discourage intermediaries from transforming the payload.
| headers={'Content-Encoding': 'identity'}, |
| ) | ||
| assert response.status_code == 200 | ||
| assert response.headers["content-type"] == "application/x-ndjson" | ||
| assert response.headers["content-encoding"] == "identity" |
There was a problem hiding this comment.
This assertion bakes Content-Encoding: identity into the public contract of the route, even though the absence of the header already implies identity. This makes the test brittle and will fail if response compression is legitimately introduced later (or if the header is removed as redundant). Consider asserting that the response is not encoded (e.g., header missing or not equal to gzip/br), or omit this assertion unless callers truly require it.
| assert response.headers["content-encoding"] == "identity" | |
| content_encoding = response.headers.get("content-encoding", "").lower() | |
| assert content_encoding not in ("gzip", "br") |
Description
Screenshots:
Checklist: