Opened 15 years ago

Closed 14 years ago

Last modified 13 years ago

#12787 closed (fixed)

TemplateDoesNotExist exception does not report the correct template_name

Reported by: trigeek38 Owned by: anonymous
Component: Template system Version: dev
Severity: Keywords: TemplateDoesNotExist
Cc: Lior Sion Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description (last modified by Karen Tracey)

When calling templateA which {% includes templateB %} that does not exist, the exception reports that templateA does not exist instead of templateB. Version (1, 1, 0, 'final', 0) reported the correct template_name.

### This is how 1.1.0 reported templates that did not exist when called by {% include %} in another template.

_order_detail.html included in order_detail.html located at /orders/650/

TemplateDoesNotExist at /orders/650/

_order_detail.html

Request Method: 	GET
Request URL:  somewhere/outside/orders/650/
Exception Type: 	TemplateDoesNotExist
Exception Value: 	

_order_detail.html

Exception Location: 	/usr/lib/python2.4/site-packages/django/template/loader.py in find_template_source, line 74
Python Executable: 	/usr/bin/python
Python Version: 	2.4.3

## This is how trunk reports the same situation. Notice here that it reports that the actual template being called by the view as not existing instead of the included template. Here I included _order_detail.html (does not exist) in po_form.html

pos/po_form.html

Request Method: 	GET
Request URL: 	somewhere/else/pos/new/
Exception Type: 	TemplateDoesNotExist
Exception Value: 	

pos/po_form.html

Exception Location: 	/home/trigeek38/lib/python2.5/django/template/loader.py in find_template, line 125
Python Executable: 	/usr/local/bin/python
Python Version: 	2.5.4
Python Path: 	['/home/trigeek38/lib/python2.5/html5lib-0.11.1-py2.5.egg', '/home/trigeek38/lib/python2.5/pisa-3.0.30-py2.5.egg', '/home/trigeek38/lib/python2.5/django_pagination-1.0.5-py2.5.egg', '/home/trigeek38/lib/python2.5', '/home/trigeek38/webapps/django_trunk/lib/python2.5', '/usr/local/lib/python25.zip', '/usr/local/lib/python2.5', '/usr/local/lib/python2.5/plat-linux2', '/usr/local/lib/python2.5/lib-tk', '/usr/local/lib/python2.5/lib-dynload', '/usr/local/lib/python2.5/site-packages', '/usr/local/lib/python2.5/site-packages/PIL', '/home/trigeek38/webapps/django_trunk/projects/', '/home/trigeek38/webapps/django_trunk/projects/']
Server time: 	Thu, 4 Feb 2010 21:43:40 -0500
Template-loader postmortem

Django tried loading these templates, in this order:

    * Using loader django.template.loaders.filesystem.Loader:
    * Using loader django.template.loaders.app_directories.Loader: 

Attachments (4)

templatefix.patch (454 bytes ) - added by jkatzer 15 years ago.
patch file to fix the bug
template patches.zip (5.5 KB ) - added by jkatzer 15 years ago.
template patches (complete)
12787.diff (5.0 KB ) - added by Karen Tracey 15 years ago.
12787.2.diff (11.1 KB ) - added by Karen Tracey 15 years ago.

Download all attachments as: .zip

Change History (18)

comment:1 by Karen Tracey, 15 years ago

Description: modified (diff)
Triage Stage: UnreviewedAccepted

(Reformatted description. Please use preview.)

comment:2 by anonymous, 15 years ago

Applying bisection shows this is a regression introduced in r11862 (cached templates feature and template loaders API refactoring). There is no need to use django.template.loaders.cached.Loader to reproduce this bug.

comment:3 by anonymous, 15 years ago

Owner: changed from nobody to jkatzer

comment:4 by jkatzer, 15 years ago

Has patch: set
Needs tests: set
Owner: changed from jkatzer to anonymous
Status: newassigned

this patch fixes it, but my testing suite is currently broken otherwise on this machine. if someone else tests it should work, if not then i will rerun tomorrow after fixing my testing suite.

the problem is that code after version 1.1 throws array the real exception and creates it own based on the templatename being used. this patch is slightly uglier, but its simple and it works.

and its my first real python code!!!
(even though its only one line)

by jkatzer, 15 years ago

Attachment: templatefix.patch added

patch file to fix the bug

comment:5 by jkatzer, 15 years ago

Needs documentation: set
Needs tests: unset

scrapped the old patch....

the new patch(es) overload the previous TemplateDoesNotExist execption, to add wether or not that error should be thrown out. it is worded as to wether or not a super function calling it "should_keep_trying"

in regards to this, any place that previously raised an TemplateDoesNotExist error needs to be rewritten to have an extra arg... this also is the case in the testing file, which has been modified accordingly...

please excuse my code as i am not fully familiar with python and the coding conventions used by the django community.

by jkatzer, 15 years ago

Attachment: template patches.zip added

template patches (complete)

by Karen Tracey, 15 years ago

Attachment: 12787.diff added

comment:6 by Karen Tracey, 15 years ago

Needs documentation: unset
Needs tests: set

jkatzer: In the future please attach a single diff file created by svn diff from the root of the tree. See: http://docs.djangoproject.com/en/dev/internals/contributing/#patch-style. That's much easier to deal with than a bunch of individual .diff files zipped up.

I'm uncomfortable with the originally proposed way of fixing this, so I've attached an alternative approach. The root cause of this bug is similar to #12992. During load_template, the load of the requested template may succeed but then get_template_from_string may raise TemplateDoesNotExist if the loaded template can't be compiled due to some other template not existing. That TemplateDoesNotExist, raised by load_template, is interpreted by its caller to mean that the requested template doesn't exist, leading to an erroneous report of the actual problem on the debug page.

Attached patch changes the loader to code to fall back to returning the template source and name, as it used to, if it was able to find the requested template but attempting to compile it raises a TemplateSyntaxError. Thus the loader will only raise TemplateSyntaxError if the specifically-requested template does not exist. That prevents this code:

    for loader in template_source_loaders:
        try:
            source, display_name = loader(name, dirs)
            return (source, make_origin(display_name, loader, name, dirs))
        except TemplateDoesNotExist:
            pass
    raise TemplateDoesNotExist(name)

from moving on and trying other loaders, then ultimately ending with TemplateDoesNotExist(name), which mis-identifies the template that does not exist.

That change alone is not sufficient to fix the problem, though. The extends node get_parent method catches TemplateDoesNotExist and turns it into a TemplateSyntaxError stating that the template specified to be extended does not exist. Prior to r11862, the call covered by the try/except was a find_template_source call, which would only raise TemplateDoesNotExist if that specific template does not exist. In r11862 the call inside the try/except was changed to get_template, which both finds the source and compiles it, and so may raise TemplateDoesNotExist if some other template needed to compile the extended template does not exist. We could either put the code here back the way it was before r11862 or remove this try/except entirely. The attached patch does the latter, because I don't really see what additional value is added by the different message ("... can't be extended because it doesn't exist") over a plain template does not exist message.

There is a test in the patch, but it's not really sufficient since it does not test both the base loader case and the cached loader case. But it does illustrate when the problem arises.

by Karen Tracey, 15 years ago

Attachment: 12787.2.diff added

comment:7 by Karen Tracey, 15 years ago

Resolution: fixed
Status: assignedclosed

(In [12792]) Fixed #12787: Correctly identify the template that does not exist when a template being extended includes another template that does not exist. Thanks to trigeek38 for the report.

comment:8 by jkatzer, 15 years ago

thanks for the advice... i was at a code sprint, and asked around for the best way to fix it and thats what we came up with....

comment:9 by Lior Sion, 14 years ago

Patch needs improvement: set
Resolution: fixed
Status: closedreopened
Triage Stage: AcceptedUnreviewed

This fix doesn't work - the original problem remains.

The original exception (found in loader.py, also in changeset:12792):

raise TemplateDoesNotExist(name) 

that's the correct line of code, however, at the same file (and same changeset:12792):

except TemplateDoesNotExist:
            # If compiling the template we found raises TemplateDoesNotExist, back off to
            # returning the source and display name for the template we were asked to load.
            # This allows for correct identification (later) of the actual template that does
            # not exist.
            return source, display_name

the exception is caught but value is not used, and so we "loose" the exception. However, since we try to render it again, it's thrown again at the same place. This time, it's caught, but there's an error there:

the original call look like this:

def select_template(template_name_list):
    "Given a list of template names, returns the first that can be loaded."
    for template_name in template_name_list:
        try:
            return get_template(template_name)
        except TemplateDoesNotExist:
            continue
    # If we get here, none of the templates could be loaded
    raise TemplateDoesNotExist(', '.join(template_name_list))

and so the exception caught there is not because the original template was not found, but because templateB was not found, but it's never recorded and identified.

A possible solution:

def select_template(template_name_list):
    "Given a list of template names, returns the first that can be loaded."
    error_list = []
    for template_name in template_name_list:
        try:
            return get_template(template_name)
        except TemplateDoesNotExist, e:
            if e.message not in template_name_list:
                error_list.append(e.message)
            continue
    # If we get here, none of the templates could be loaded
    template_name_list += error_list
    raise TemplateDoesNotExist(', '.join(template_name_list))

we need to make sure the reason for the TemplateDoesNotExist exception is not "just" looking for TemplateA in a wrong directory (the external loop).

A different approach would be to create SubTemplateDoesNotExist exception type.

comment:10 by Lior Sion, 14 years ago

Cc: Lior Sion added

comment:11 by Karen Tracey, 14 years ago

Resolution: fixed
Status: reopenedclosed

The problem identified in this ticket was fixed, really. The fix includes tests, which failed before the code change and are still passing on current trunk:

~/django/trunk/tests
--> ./runtests.py --settings=testdb.sqlite -v2 templates.Templates.test_extends_include_missing_baseloader templates.Templates.test_extends_include_missing_cachedloader
Importing application templates
Creating test database for alias 'default' (':memory:')...
Creating tables ...
Creating table django_content_type
Creating table auth_permission
Creating table auth_group_permissions
Creating table auth_group
Creating table auth_user_user_permissions
Creating table auth_user_groups
Creating table auth_user
Creating table auth_message
Creating table django_site
Creating table django_flatpage_sites
Creating table django_flatpage
Creating table django_redirect
Creating table django_session
Creating table django_comments
Creating table django_comment_flags
Creating table django_admin_log
Installing custom SQL ...
Installing indexes ...
No fixtures found.
Creating test database for alias 'other' ('other_db')...
Destroying old test database 'other'...
Creating tables ...
Creating table django_content_type
Creating table auth_permission
Creating table auth_group_permissions
Creating table auth_group
Creating table auth_user_user_permissions
Creating table auth_user_groups
Creating table auth_user
Creating table auth_message
Creating table django_site
Creating table django_flatpage_sites
Creating table django_flatpage
Creating table django_redirect
Creating table django_session
Creating table django_comments
Creating table django_comment_flags
Creating table django_admin_log
Installing custom SQL ...
Installing indexes ...
No fixtures found.
test_extends_include_missing_baseloader (regressiontests.templates.tests.Templates) ... ok
test_extends_include_missing_cachedloader (regressiontests.templates.tests.Templates) ... ok

----------------------------------------------------------------------
Ran 2 tests in 0.004s

OK
Destroying test database for alias 'default' (':memory:')...
Destroying test database for alias 'other' ('other_db')...

Further, if you doubt the tests, if I change one of my current projects to have the problem noted in the original description -- one existing template including a 2nd existing template that in turn attempts to include a 3rd template that does not exist, the debug page I get correctly identifies the template that is missing.

You may be seeing a bug, but it is not this exact bug. Better than trying to show how the code change made here is wrong would be a test case that shows the case you are running into where a problem exists. And that should go into its own new ticket (that perhaps references this one), because this specific one is fixed.

in reply to:  11 comment:12 by Lior Sion, 14 years ago

The issue that was fixed here + the tests are for the wrong thing: an extended template that is missing an include, and not a normal template that is missing an include - the tests are also testing a different scenario.

The tested template has:

{% extends "broken_base.html" %}

while it should have:

{% include "just/something.html" %}

comment:13 by Karen Tracey, 14 years ago

Re-reading now, I find the original description inconclusive as to which case it was reporting -- depends on what was meant by "call": render or extend. I don't remember if I tried both while initially looking at the ticket and choose to focus on the extends case because it was a superset of the other, or if I just assumed "call" meant extend. In some brief tests now with running against r12791/r12792 (right before and after the fix) if I switch things around to be just a plain include of a missing template, I see the wrong template reported before the fix and the correct one after the fix, so the case you mention here was also fixed by this code change. I do also believe that case is a subset of the extends case that is actually tested, so I don't believe either the tests or the fix here are "wrong".

Perhaps the problem you are encountering is the one identified in #15502, which involves a similar problem when a list of templates is provided. Sorry, I did not consider that case when looking at this ticket (there was no mention of lists of templates being tried), so that case was not fixed by this ticket.

comment:14 by Jacob, 13 years ago

milestone: 1.2

Milestone 1.2 deleted

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