#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 , 19 months ago
Has patch: | set |
---|
comment:2 by , 19 months 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 , 19 months 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 , 18 months 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 , 18 months ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
Triage Stage: | Accepted → Ready for checkin |
Created https://github.com/django/django/pull/16844