Opened 11 months ago

Closed 11 months ago

Last modified 11 months ago

#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 Alexandre Spaeth, 11 months ago

Has patch: set

comment:2 by Natalia Bidart, 11 months ago

Resolution: invalid
Status: newclosed

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 Alexandre Spaeth, 11 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.

Last edited 11 months ago by Alexandre Spaeth (previous) (diff)

comment:4 by Natalia Bidart, 11 months ago

Resolution: invalid
Status: closednew
Triage Stage: UnreviewedAccepted

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 Natalia Bidart, 11 months ago

Owner: changed from nobody to Alexandre Spaeth
Status: newassigned
Triage Stage: AcceptedReady for checkin

comment:6 by GitHub <noreply@…>, 11 months ago

Resolution: fixed
Status: assignedclosed

In 599f3e2c:

Fixed #34556 -- Doc'd that StreamingHttpResponse accepts memoryviews and strings iterators.

comment:7 by Natalia <124304+nessita@…>, 11 months ago

In ddccece:

[4.2.x] Fixed #34556 -- Doc'd that StreamingHttpResponse accepts memoryviews and strings iterators.

Backport of 599f3e2cda50ab084915ffd08edb5ad6cad61415 from main

Note: See TracTickets for help on using tickets.
Back to Top