Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#18764 closed Bug (fixed)

HttpResponse._get_content shouldn't accept non-text types

Reported by: Aymeric Augustin Owned by: Aymeric Augustin
Component: HTTP handling Version: master
Severity: Release blocker 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


Currently, the httpwrappers tests verify the behavior of constructs such as HttpResponse([1,1,2,4,8]). This doesn't make any sense to me.

HttpResponse must convert its content to bytes. But when a Content-Encoding is already set, it mustn't perform any encoding, so it can't go through a unicode representation.

Now, in Python 3, bytes(3) == \x00\x00\x00. So I don't see any way to implement the construct above in Python 3 without special-casing integers.

Rather than explicitly hacking a meaningless behavior, I'd like to remove these tests and require that HttpResponse be instantiated with a (byte)string or an iterable of (byte)strings.

I'm marking this as a release blocker because it causes a test failure under Python 3 (httpwrappers.HttpResponseTests.test_iter_content).

Attachments (1)

18764.patch (2.7 KB) - added by Aymeric Augustin 4 years ago.

Download all attachments as: .zip

Change History (7)

comment:1 Changed 4 years ago by Claude Paroz

Triage Stage: UnreviewedAccepted

+1, I tried more than once to redesign HttpResponse and found that current tests make this hard, so I fully concur on this issue.

comment:2 Changed 4 years ago by Karen Tracey

Note #16494 also dealt with HttpResponse behavior when handed non-string types, and was fixed by converting to string representation. The fact that someone hit that hit that problem makes me fear people may be using this behavior even if they don't realize it. Couldn't we convert non-text types to unicode and encode (using the encoding specified, if it has been specified)? Or at least continue to support the existing behavior for those cases where the app hasn't messed with the response encoding?

comment:3 Changed 4 years ago by Aymeric Augustin

Owner: changed from nobody to Aymeric Augustin

Karen, thanks for pointing me to that ticket.

I'll review that discussion and implement a consistent solution — it isn't hard.

Changed 4 years ago by Aymeric Augustin

Attachment: 18764.patch added

comment:4 Changed 4 years ago by Aymeric Augustin

Has patch: set
Triage Stage: AcceptedReady for checkin

I've just attached a patch that's consistent with the tickets I could find on this topic.

force_bytes might be a bit overkill here but it has the following advantage:

>>> bytes(10)
>>> from django.utils.encoding import smart_bytes
>>> smart_bytes(10)

Allowing ASCII encoding when a Content-Encoding is set is still correct with regard to #4969: Python 2 allowed itself to perform str <=> unicode conversions of ASCII data transparently, making that explicit doesn't hurt.

comment:5 Changed 4 years ago by Aymeric Augustin <aymeric.augustin@…>

Resolution: fixed
Status: newclosed

In [536b0303637c55c914784b57841e6d59c4968d5f]:

[py3] Supported integers in HttpResponse

Fixed #18764.

comment:6 Changed 4 years ago by Aymeric Augustin <aymeric.augustin@…>

In [2892cb0ec4ebde4955f12c92183b7804558ca381]:

[py3] Fixed regression introduced in 536b030363.

Refs #18764.

Reverted 536b030363 and switched to a more explicit way of avoiding
calling bytes(<int>).

This definitely deserves a refactoring. Specifically, _get_content
should just return b.join(self). Unfortunately that's impossible
with the current tests.

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