Code

Opened 2 years ago

Closed 23 months ago

Last modified 23 months ago

#18764 closed Bug (fixed)

HttpResponse._get_content shouldn't accept non-text types

Reported by: aaugustin Owned by: aaugustin
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

Description

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 aaugustin 23 months ago.

Download all attachments as: .zip

Change History (7)

comment:1 Changed 2 years ago by claudep

  • Triage Stage changed from Unreviewed to Accepted

+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 2 years ago by kmtracey

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 2 years ago by aaugustin

  • Owner changed from nobody to aaugustin

Karen, thanks for pointing me to that ticket.

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

Changed 23 months ago by aaugustin

comment:4 Changed 23 months ago by aaugustin

  • Has patch set
  • Triage Stage changed from Accepted to Ready 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)
b'\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00'
>>> from django.utils.encoding import smart_bytes
>>> smart_bytes(10)
b'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 23 months ago by Aymeric Augustin <aymeric.augustin@…>

  • Resolution set to fixed
  • Status changed from new to closed

In [536b0303637c55c914784b57841e6d59c4968d5f]:

[py3] Supported integers in HttpResponse

Fixed #18764.

comment:6 Changed 23 months 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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.