__getattr__ on SimpleTemplateResponse causes problems
Luke Plant
r16568 [1] added SimpleTemplateResponse.__getattr__ as part of a fix for
#16326 [2].
There is one obvious bug in the implementation - it calls
super(...).__getattr__ which itself raises AttributeError because none
of the base classes have __getattr__. It should use getattr instead.
That aside, the existence of this __getattr__ is very unfortunate. It
means that if you have any AttributeError that escapes when a
SimpleTemplateResponse is rendered, the .rendered_content property
releases an AttributeError, confusing Python into thinking that
'rendered_content' is not an attribute, and so it uses the __getattr__
to look for the attribute. Whatever happens from this point, your
original AttributeError is swallowed, and this means you lose all the
lovely debugging goodness that Django provides.
This part of r16568 does not seem at all necessary for fixing the
original bug. Rather, it simply gives a nicer error message if your code
accesses the attributes that were removed when a SimpleTemplateResponse
was pickled. In fact, even that doesn't work correctly if your code is
running as a result of a SimpleTemplateResponse being rendered, due to
the problem already described - and in this case you would instead get
this message:
"The rendered_content attribute was discarded when this
TemplateResponse class was pickled."
which is completely false.
Since debugging of code that runs in SimpleTemplateResponse is very
common, and AttributeError is very common, and accessing attributes of
unpickled SimpleTemplateResponses is fairly uncommon, I would like to
revert this part of the patch, and the associated tests, both of which
go well beyond the scope of the original bug.
Objections or comments?
Thanks,
Luke
[1] https://code.djangoproject.com/changeset/16568
[2] https://code.djangoproject.com/ticket/16326
--
I teleported home one night
With Ron and Sid and Meg,
Ron stole Meggie's heart away
And I got Sidney's leg
(THHGTTG)
Luke Plant || http://lukeplant.me.uk/
Luke Plant
> and in this case you would instead get
> this message:
>
> "The rendered_content attribute was discarded when this
> TemplateResponse class was pickled."
>
> which is completely false.
...which is completely false, so please ignore that part :-) The rest of
the email was sound though, I think, and the problem bit a few times
over the past few days.
Luke
Ramiro Morales
> Hi all,
>
> r16568 [1] added SimpleTemplateResponse.__getattr__ as part of a fix for
> #16326 [2].
>
> There is one obvious bug in the implementation - it calls
> super(...).__getattr__ which itself raises AttributeError because none
> of the base classes have __getattr__. It should use getattr instead.
#16935[1] reports that, I think.
https://code.djangoproject.com/ticket/16935
--
Ramiro Morales
Florian Apolloner
On Friday, September 30, 2011 2:22:12 AM UTC+2, Ramiro Morales wrote:
On Thu, Sep 29, 2011 at 8:43 PM, Luke Plant <L.Pla...@cantab.net> wrote:
> Hi all,
>
> r16568 [1] added SimpleTemplateResponse.__getattr__ as part of a fix for
> #16326 [2].
>
> There is one obvious bug in the implementation - it calls
> super(...).__getattr__ which itself raises AttributeError because none
> of the base classes have __getattr__. It should use getattr instead.#16935[1] reports that, I think.
Yes, and it's super annoying, so I am +1 for everything which fixes it :)
Ivan Sagalaev
Jannis Leidel
On 30.09.2011, at 01:58, Luke Plant wrote:
> I wrote:
>
>> and in this case you would instead get
>> this message:
>>
>> "The rendered_content attribute was discarded when this
>> TemplateResponse class was pickled."
>>
>> which is completely false.
>
> ...which is completely false, so please ignore that part :-) The rest of
> the email was sound though, I think, and the problem bit a few times
> over the past few days.
I was about to ask, that was a bit confusing.
That said, the rest of your analysis is as always striking, +1.
Jannis