Opened 13 years ago

Closed 13 years ago

Last modified 10 years ago

#16770 closed Bug (fixed)

Don't wrap exceptions in TemplateSyntaxError under DEBUG

Reported by: jMyles Owned by: jMyles
Component: Template system Version: 1.3
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

This problem is easy to reproduce:

*Create a new django project.
*In settings.py, set the database to postgres but with a non-existent hostname.
*Create a model (Llama) and view (home)
*In the view, pass Llama.objects.all() to the context as "llamas"
*In the template, attempt to iterate through "llamas"

Attachments (7)

Screenshot-1.png (8.4 KB ) - added by jMyles 13 years ago.
Shot of the actual error
better_operationalerror.diff (782 bytes ) - added by jMyles 13 years ago.
Not a final patch per se, just a first draft to get the ball rolling. This simply causes OperationalError to be raised instead of TemplateSyntaxError.
unwrap_templatesyntaxerror.diff (7.4 KB ) - added by jMyles 13 years ago.
OK, this patch is feeling a little more final. I have some concerns about the fact that there seems to be no branch whatsoever that causes execution of the except block of line 79 of the template/debug. I'll get confirmation.
unwrap_templatesyntaxerror-1.diff (17.9 KB ) - added by jMyles 13 years ago.
Here's the latest patch. In addition to the code and tests, this one also fixes old tests that expected TemplateSyntaxError.
unwrap_templatesyntaxerror-2.diff (17.9 KB ) - added by jMyles 13 years ago.
Removed psycopg2 import
unwrap_templatesyntaxerror-3.txt (17.5 KB ) - added by jMyles 13 years ago.
Another correction: The first modification wasn't intentional, it was just regression from failing to svn update.
unwrap_templatesyntaxerror-3.diff (17.5 KB ) - added by jMyles 13 years ago.

Download all attachments as: .zip

Change History (14)

by jMyles, 13 years ago

Attachment: Screenshot-1.png added

Shot of the actual error

comment:1 by jMyles, 13 years ago

It looks to me like the core of the problem is template/debug.py line 79.

by jMyles, 13 years ago

Not a final patch per se, just a first draft to get the ball rolling. This simply causes OperationalError to be raised instead of TemplateSyntaxError.

comment:2 by jMyles, 13 years ago

On advice from Carl, I have decided to unwrap the exceptions altogether. Instead, we're going to annotate the exception, passing the node (and thus the line of the template) that was rendering when the error occurred. This way, nobody will be mystified about what part of the template is hitting the database and causing OperationalError or DatabaseError.

by jMyles, 13 years ago

OK, this patch is feeling a little more final. I have some concerns about the fact that there seems to be no branch whatsoever that causes execution of the except block of line 79 of the template/debug. I'll get confirmation.

comment:3 by Carl Meyer, 13 years ago

Summary: TemplateSyntaxError is raised improperly in some cases where database connection failsDon't wrap exceptions in TemplateSyntaxError under DEBUG
Triage Stage: UnreviewedAccepted

jMyles: this patch is looking great! Apart from some minor stylistic things which can easily be tweaked at commit time (e.g. use double-quotes not single-quotes for delimiting docstrings, and some of those comments are probably not needed at all), I just notice one substantive issue. Now that we aren't exclusively annotating TemplateSyntaxError, but any kind of exception, there's no reason to have two separate clauses in DebugNodeList.render_node, one to catch TemplateSyntaxError and one for other exceptions. There should just be a single clause for all exceptions, and it should check to make sure the annotation doesn't already exist on the exception before adding it.

Also, I'm not sure why we need the complex multi-argument re-raise anymore - ExceptionReporter should get the traceback automatically, because its getting the original exception.

by jMyles, 13 years ago

Here's the latest patch. In addition to the code and tests, this one also fixes old tests that expected TemplateSyntaxError.

by jMyles, 13 years ago

Removed psycopg2 import

comment:4 by Carl Meyer, 13 years ago

Thanks jMyles. I've done some work on the patch, I think it's pretty close to ready. Current state here: https://github.com/carljm/django/compare/master...t16770

by jMyles, 13 years ago

Another correction: The first modification wasn't intentional, it was just regression from failing to svn update.

by jMyles, 13 years ago

comment:5 by jMyles, 13 years ago

The corrections I made in unwrap_templatesyntaxerror-3.diff are also in the github - it's now the authoritative version of this patch: https://github.com/carljm/django/compare/master...t16770

comment:6 by Carl Meyer, 13 years ago

Resolution: fixed
Status: newclosed

In [16833]:

Fixed #16770 -- Eliminated TemplateSyntaxError wrapping of exceptions. Thanks to Justin Myles-Holmes for report and draft patch.

Exceptions raised in templates were previously wrapped in TemplateSyntaxError
(in TEMPLATE_DEBUG mode only) in order to provide template source details on
the debug 500 page. The same debug information is now provided by annotating
exceptions rather than wrapping them. This makes catching exceptions raised
from templates more sane, as it's consistent in or out of DEBUG, and you can
catch the specific exception(s) you care about rather than having to also catch
TemplateSyntaxError and unwrap it.

comment:7 by Aymeric Augustin, 10 years ago

#12995 was a duplicate.

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