[Django] #31478: Template.get_exception_info relies on internal state which isn't correct for sub-templates.

15 views
Skip to first unread message

Django

unread,
Apr 18, 2020, 12:36:39 PM4/18/20
to django-...@googlegroups.com
#31478: Template.get_exception_info relies on internal state which isn't correct
for sub-templates.
-------------------------------------------+------------------------
Reporter: Keryn Knight | Owner: nobody
Type: Bug | Status: new
Component: Template system | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------------+------------------------
Currently, the method is defined as:
{{{
def get_exception_info(self, exception, token):
}}}
and internal to it's scope, makes use of the values `self.source` and
`self.origin.name`

These 2 variables are not necessarily the data on which the operations
should apply, at least in some scenarios. I have a demo project while I'll
attach to hopefully demonstrate this.

The problem exists on 2.2, 3.0 & master (being 3.1 currently). Whilst I've
not done an exhaustive sweep of the potential problems, a solution which
seems to work in my limited test is thus:

Method signature becomes
{{{
def get_exception_info(self, exception, token, origin=None, source=None):
if source is None:
source = self.source
if origin is None:
origin = self.origin
}}}
Why support Nones? Because at the very least I think django-debug-toolbar
makes use of the method, and it leaves it backwards compatible.

The callers I'm aware of are
`django.template.base.Template.compile_nodelist`, which would become:
{{{
e.template_debug = self.get_exception_info(e, e.token, self.origin,
self.source)
}}}
though I think that given that is passing in the *same* data, it could
actually remain unchanged, because the internal state is the same both
ways.

... and `django.template.base.Node.render_annotated`:
{{{
e.template_debug = context.render_context.template.get_exception_info(e,
self.token, context.template.origin, context.template.source)
}}}
This one is what I presume is the crux of the problem, because the render
context's template is not the same as the child template.

Of course, changing the origin and source might have other knock on
effects that I've not encountered in my tests, and I haven't tried the
proposed changes against the test suite etc.

--
Ticket URL: <https://code.djangoproject.com/ticket/31478>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

Django

unread,
Apr 18, 2020, 12:44:28 PM4/18/20
to django-...@googlegroups.com
#31478: Template.get_exception_info relies on internal state which isn't correct
for sub-templates.
---------------------------------+--------------------------------------

Reporter: Keryn Knight | Owner: nobody
Type: Bug | Status: new
Component: Template system | Version: master
Severity: Normal | Resolution:

Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+--------------------------------------

Comment (by Keryn Knight):

Example project is here: https://github.com/kezabelle/django-ticket-31478

The 2 dependencies are Django and the app with which I encountered the
problem, which is itself a terrible set of monkeypatches upon the Django
Template Language. There's probably cleaner ways to demonstrate the issue,
but this is the quickest one I obviously have at hand. As far as I know
it's not specifically the monkeypatch at fault, but I could be wrong.

Steps to hopefully reproduce:
- clone repo
- install requirements in root of repo (`pip install -r requirements.txt`)
- boot runserver.
- navigate your browser to `/fine` with DEBUG turned on, so the technical
500 renders. You should hopefully observe that the template's HTML is
shown, and the error is ''highlighted correctly'' as `{{ force_error }}`
- navigate to `/problem` to see the error being described in the ticket
proper. You should hopefully observe that the HTML shown is that of the
`base.html` template rather than the `subtemplate.html` which **extends**
`base.html` - as a result, ''the HTML is wrong, there is no line to
highlight, and it reports the error at line 0''.

With the proposed possible changes, the error is highlighted correctly on
`/problem`, the line is reported as line 4 correctly, and the path to the
template shown is the correct subtemplate rather than the base one.

--
Ticket URL: <https://code.djangoproject.com/ticket/31478#comment:1>

Django

unread,
Apr 18, 2020, 9:15:59 PM4/18/20
to django-...@googlegroups.com
#31478: Template.get_exception_info relies on internal state which isn't correct
for sub-templates.
---------------------------------+--------------------------------------

Reporter: Keryn Knight | Owner: nobody
Type: Bug | Status: new
Component: Template system | Version: master
Severity: Normal | Resolution:

Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+--------------------------------------

Comment (by Tim Graham):

Duplicate of #28935?

--
Ticket URL: <https://code.djangoproject.com/ticket/31478#comment:2>

Django

unread,
Apr 20, 2020, 4:28:16 AM4/20/20
to django-...@googlegroups.com
#31478: Template.get_exception_info() relies on internal state which isn't correct
for sub-templates.
---------------------------------+--------------------------------------

Reporter: Keryn Knight | Owner: nobody
Type: Bug | Status: closed

Component: Template system | Version: master
Severity: Normal | Resolution: duplicate

Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+--------------------------------------
Changes (by felixxm):

* status: new => closed
* resolution: => duplicate


Comment:

Thanks Keryn, I think we can mark this as a duplicate of #28935 (as Tim
suggested). It's a regression in e643ba8bcf0b1da517cbab689ac157ee031202a3
and your patch restores the previous behavior. Unfortunately it also
causes two failures:
{{{
======================================================================
FAIL: test_compile_tag_error_27584 (template_tests.tests.TemplateTests)
----------------------------------------------------------------------
...
AssertionError: '' != '{% badtag %}'
+ {% badtag %}
======================================================================
FAIL: test_compile_tag_error_27956 (template_tests.tests.TemplateTests)
Errors in a child of {% extends %} are displayed correctly.
----------------------------------------------------------------------
...
AssertionError: 'nt.html&quot; %}\n' != '{% badtag %}'
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/31478#comment:3>

Reply all
Reply to author
Forward
0 new messages