#34556 closed Uncategorized (fixed)
StreamingHttpResponse documentation inaccuracy
| Reported by: | Alexandre Spaeth | Owned by: | Alexandre Spaeth |
|---|---|---|---|
| Component: | Documentation | Version: | 4.2 |
| Severity: | Normal | Keywords: | |
| Cc: | Triage Stage: | Ready for checkin | |
| Has patch: | yes | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | no |
| Easy pickings: | no | UI/UX: | no |
Description
While working on adding typing in django-stubs for the async version of StreamingHttpResponse, we noticed that the documentation for StreamingHttpResponse mentions
It should be given an iterator that yields bytestrings as content.
But reading at the code (and the tests), it’s clear that the content can actually also be a string (or anything that can be converted to bytestrings by make_bytes). If that’s the expected behaviour (and I’m assuming it is, looking at the content documentation for HttpResponse), I suggest we fix the documentation.
Change History (7)
comment:1 by , 2 years ago
| Has patch: | set |
|---|
comment:2 by , 2 years ago
| Resolution: | → invalid |
|---|---|
| Status: | new → closed |
Hello Alexandre!
I believe we'd have to close this ticket as invalid, see my reasoning below:
On the one hand, a string *is* an iterator, so strictly speaking, a string can be passed to StreamingHttpResponse because a string is an iterable on its own.
On the other hand, the usage of make_bytes in streaming_content is focused on ensuring that the part being returned is, in fact, bytes and not other type.
Lastly, the documentation clearly says that StreamingHttpResponse is not a subclass of HttpResponse so concepts that apply to the latter would not apply to the former. Specifically, a StreamingHttpResponse has no content attibute as described here.
What tests are you referring to that pass a string to StreamingHttpResponse?
Based on the above and after re-reading the docs for StreamingHttpResponse, I think that the current text is correct and complete so I'll close as invalid but please re-open if I misread or misunderstood your report.
comment:3 by , 2 years ago
Hello Natalia, I guess I wasn’t clear enough :-)
I meant "the content of the iterator", not "the content of the response", my bad.
More clearly:
The streaming_content passed to the class constructor is an Iterable[bytes] (or an AsyncIterable[bytes]), but according to https://github.com/django/django/blob/6e32d1fa1dafd0c9cd9f93997ecebb26cd9a1b62/tests/httpwrappers/tests.py#L672, it can also be an Iterable[str] or even Iterable[object] in the case of a memoryview (and their respective async ones).
The documentation, as written now, assumes that only Iterable[bytes] are acceptable, which is not the case.
I totally agree about the fact that the streaming_content is an iterable of bytes though and wouldn’t touch this.
comment:4 by , 2 years ago
| Resolution: | invalid |
|---|---|
| Status: | closed → new |
| Triage Stage: | Unreviewed → Accepted |
Thanks for the clarification, I now understand your point. Accepting then following the linked examples and the implementation code that in fact allows for iterator of: bytes, memoryviews, strings or other str-convertable types.
comment:5 by , 2 years ago
| Owner: | changed from to |
|---|---|
| Status: | new → assigned |
| Triage Stage: | Accepted → Ready for checkin |
Created https://github.com/django/django/pull/16844