Opened 5 years ago

Closed 5 years ago

Last modified 4 years ago

#15025 closed (fixed)

Different template rendering semantics since r14992

Reported by: donspaulding Owned by: nobody
Component: Template system Version: master
Severity: Keywords: blocker, regression
Cc: mb0087@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

Changeset [14992] fixed Ticket #7153, but I believe it also changed the way callable context variables are resolved in the Django template language in a subtle (and backwards-incompatible) way.

Consider the following view, which raises an unhandled exception while trying to render the debug 500 template:

def view(request):
    def innocent_callable():
        raise Exception("Should not have been called")
    raise Exception("This exception should trigger a debug 500 page")

I've already found two regressions in my own code, but they were simple workarounds. This example appears to be a regression in the django source itself. I'm not sure if this change in behavior was anticipated or intended, but at the very least I think it should be noted in the release notes as a backwards-incompatibility.

Attachments (1)

15025-technical-500-response-r15153.diff (2.0 KB) - added by mrmachine 5 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 5 years ago by ramiro

  • Keywords blocker regression added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

I've also seen this while working on #11715, the problem can also be reproduced with the test case posted by the reporter there.

One where this problem seems to be happening is rendering of the page when 500 error is raised and if DEBUG is True, some callables are run that weren't before, and that generates other exception because at that point classes are instantiated that aren't supposed to be (e.g. ModelForms).

If ones goes back to r14991 the 500 usual error page is generated reporting the correct error and showing an useful traceback.

There have been one report in the django-user list as well:

https://groups.google.com/d/topic/django-users/8TGK7PvDD3Y/discussion

comment:2 Changed 5 years ago by anonymous

  • Cc mb0087@… added

I'm not sure but I guess my case is also related to these changes.
I get the error: 'ModelForm' object has no attribute '_meta caused by a {% url profile_overview %} in my template.
Full traceback here: http://dpaste.com/hold/294818/

Havent figured out why, but its not there if I go back to r14991.
More details here:
http://groups.google.com/group/django-users/browse_thread/thread/f1318aecfbc30f76/b16ca41934f76518#b16ca41934f76518

comment:3 Changed 5 years ago by kmtracey

I think #15034 reported this again with a patch w/ test for the debug page. The description here makes me think problems due to this change may be more widespread than just the debug page?

comment:4 Changed 5 years ago by mrmachine

  • milestone set to 1.3

Looking at [14992], I believe that the problem is only on the debug page.

When rendering normal templates, any template variable resolution that raises an exception *should* raise it loudly if the exception doesn't explicitly have an attribute silent_variable_failure which is True. The exception should then be displayed on the debug page or emailed to the developer, etc.

The problem now with the debug page is that it is resolving local vars for each stack frame as template vars, which transforms the variable in the case of a callable. Besides possibly raising exceptions and crashing the debug page, this is incorrect behaviour for the debug page which should only be showing the __repr__ for stack frame local vars, not evaluating and transforming them.

For example, if your view has a variable form_class which is a subclass of django.forms.Form, the debug page should display this as a form class and not as an unbound form instance.

Changed 5 years ago by mrmachine

comment:5 Changed 5 years ago by mrmachine

  • Has patch set

Moved patch from #15034 to here, as I still think it solves the issue (with test).

comment:6 Changed 5 years ago by donspaulding

  • Needs tests set
Looking at [14992], I believe that the problem is only on the debug page.

The debug page is representative of third-party application templates in that it was relying on the semantics of context variable resolution. It's incredibly hard to imagine all the ways people will (ab)use the template language. I haven't written enough templates to know if there's a way to enable resolution of DictMixin instance methods (the goal of #7153) without impacting other uses of callables within templates. My hunch is that there isn't, but I'd be happy to be proven wrong.

comment:7 Changed 5 years ago by mrmachine

  • Needs tests unset

The debug page is not representative of third party application templates.

The debug page is special because it is an error handling function of the core framework (not a normal application view/template) that exists to show the details surrounding an unhandled exception in your code. It is inappropriate for the generation of the technical 500 response to actually raise it's own exception, and allowing potentially unsafe context to be passed to the debug page template and evaluated there is a bug in the debug page view, that can result in a hijacked response and a raw traceback in your browser.

Normal third party application templates don't do that, and if they do have a special error handling response that performs a similar function to the debug page, it would be their responsibility to pass safe context data into their template like we do (with this patch).

Previously, a template variable that was a dict which had a callable item would return the callable item when resolved. I think that was an unintentional bug, fixed in [14992]. Now, callable dict items are called just like callable object attributes, which is more consistent and the right thing to do in the context of normal template variable resolution. In normal templates, if any callable raises an exception that doesn't explicitly fail silently, it should raise an exception loudly (which triggers the debug page), but this behaviour is not appropriate for the debug page.

I'm removing the "needs tests" flag because you don't specify what else you feel needs to be or could be tested. I think this is a bug with the debug page that has always been there (passing in and evaluating potentially unsafe context), but has been exposed by the changes in [14992], and we have a test for that already. Even in the case where no exception it raised, it's incorrect to display the evaluated context in the local vars section of the debug page. The debug page should only describe the local vars of each frame in the trace stack. It shouldn't try to evaluate them. Normal templates on the other hand should try to evaluate normal template vars.

comment:8 Changed 5 years ago by anonymous

I can confirm that the patch fixed the problem for my case. Thank you :)

comment:9 Changed 5 years ago by donspaulding

...I think that was an unintentional bug...

Here's where we disagree. I think [14992] added a feature, rather than fixing a bug (how it is marked in Trac notwithstanding). Based on that premise, this ticket will need tests to assure that attribute lookup on callables works again.

Rather than trying to decide that in these comments, I'm going to take this discussion over to django-dev to see how far out in left field I am.

comment:10 Changed 5 years ago by mrmachine

See #15057 for a docs patch that describes the new behaviour introduced in [14992].

comment:11 Changed 5 years ago by ambv

I encountered this bug as well (on improperly configured inlines for the admin) and the patch fixed it for me. Now it correctly shows a debug screen.

comment:12 Changed 5 years ago by lukeplant

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

(In [15187]) Fixed #15025 - template debug fails if there's a callable local var that generates an exception

Thanks to Tai Lee for the patch and report, also to Don Spaulding.

comment:13 Changed 5 years ago by lukeplant

(In [15188]) Fixed #15057 - documented change in [14992]

Thanks to Tai Lee for the patch.

Refs #15025, #7153

comment:14 Changed 5 years ago by donspaulding

comment:15 Changed 4 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

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