Re: HttpResponse and non-string content values

[Anssi Kääriäinen]

On Aug 19, 9:35 pm, Paul McMillan  wrote:
> I replied on the ticket, but for the record here as well, the issue of
> consuming the iterator multiple times is tracked in #7581. I don't
> think that issue needs to be a blocker for this one, since it has
> existed for quite some time now.
>
> https://code.djangoproject.com/ticket/7581

By the way, it would be really good to fix that issue. The quantum
state of response.content (you change it by measuring it) is a bit
surprising to say the least. For example, try this in IPython:

>>> r = HttpResponse(iter(['a', 'b']))
>>> r.content
''
>>> r2 = HttpResponse(iter(['a', 'b']))
>>> print r2.content
'ab'
>>> r3 = HttpResponse(iter(['a', 'b']))
>>> r3.content == r3.content
False

That might be breaking some Python conventions.

Currently, a third party Middleware writer has zero chance to get the
logic right on first try if he doesn't know about this gotcha. If he
needs the content of a response, he will supposedly use the
response.content attribute. And not test what happens when the content
is an iterator. There is no mention of this issue in "writing your own
middleware" section of Django documentation.

 - Anssi

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-developers@googlegroups.com.
To unsubscribe from this group, send email to 
django-developers+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-developers?hl=en.

Re: HttpResponse and non-string content values

[Jacob Kaplan-Moss]

On Thu, Aug 18, 2011 at 5:25 PM, Paul McMillan  wrote:
> I added some test cases and cleaned the code up a bit. I believe sure
> this solution is backwards compatible (we're adding the string
> conversion, something that shouldn't have been possible before), but
> I'd like input from some other people.

Looks right to me. I tried to come up with corner cases that wouldn't
be backwards-compatible, but couldn't. Even if someone's got one, the
previous behavior was arguably a bug anyway: a property set in
__init__ shouldn't behave differently than one set later.

Jacob

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-developers@googlegroups.com.
To unsubscribe from this group, send email to 
django-developers+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-developers?hl=en.

Re: HttpResponse and non-string content values

[Paul McMillan]

I replied on the ticket, but for the record here as well, the issue of
consuming the iterator multiple times is tracked in #7581. I don't
think that issue needs to be a blocker for this one, since it has
existed for quite some time now.

https://code.djangoproject.com/ticket/7581

-Paul

On Fri, Aug 19, 2011 at 2:24 AM, Anssi Kääriäinen
 wrote:
> On Aug 19, 1:25 am, Paul McMillan  wrote:
>> I added some test cases and cleaned the code up a bit. I believe sure
>> this solution is backwards compatible (we're adding the string
>> conversion, something that shouldn't have been possible before), but
>> I'd like input from some other people.
>>
>> -Paul
>>
>> [1]https://code.djangoproject.com/ticket/16494
>
> I took a look of the patch. The patch is backwards compatible, but the
> problem is that I don't think the current implementation is correct.
> It consumes the iterator multiple times. See the comments in the
> ticket for details.
>
>  - Anssi
>
> --
> You received this message because you are subscribed to the Google Groups 
> "Django developers" group.
> To post to this group, send email to django-developers@googlegroups.com.
> To unsubscribe from this group, send email to 
> django-developers+unsubscr...@googlegroups.com.
> For more options, visit this group at 
> http://groups.google.com/group/django-developers?hl=en.
>
>

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-developers@googlegroups.com.
To unsubscribe from this group, send email to 
django-developers+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-developers?hl=en.

Re: HttpResponse and non-string content values

[Anssi Kääriäinen]

On Aug 19, 1:25 am, Paul McMillan  wrote:
> I added some test cases and cleaned the code up a bit. I believe sure
> this solution is backwards compatible (we're adding the string
> conversion, something that shouldn't have been possible before), but
> I'd like input from some other people.
>
> -Paul
>
> [1]https://code.djangoproject.com/ticket/16494

I took a look of the patch. The patch is backwards compatible, but the
problem is that I don't think the current implementation is correct.
It consumes the iterator multiple times. See the comments in the
ticket for details.

 - Anssi

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-developers@googlegroups.com.
To unsubscribe from this group, send email to 
django-developers+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-developers?hl=en.

HttpResponse and non-string content values

[Paul McMillan]

I wrote a patch for #16494 [1] that could use some a few more eyes.

In short, the issue here is that the existing code for setting the
content of an HTTPResponse behaves inconsistently when given
non-strings. Setting content during __init__ behaves differently for
iterators than setting it later via the content property. If a
non-string, non-iterator is set for content, the code eventually
throws an error during string operations. If content is an iterator,
it is iterated over and (crucially) the content is converted to a
string.

My initial response was to fix the logic error for non-strings and
return an explicit error stating that this function requires a string
for content. However, since the existing iterator code does convert to
a string, the most consistent thing we can do here seems to be
converting everything (including non-iterators) right before output.

I added some test cases and cleaned the code up a bit. I believe sure
this solution is backwards compatible (we're adding the string
conversion, something that shouldn't have been possible before), but
I'd like input from some other people.

-Paul

[1] https://code.djangoproject.com/ticket/16494

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To post to this group, send email to django-developers@googlegroups.com.
To unsubscribe from this group, send email to 
django-developers+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-developers?hl=en.