Opened 12 years ago

Closed 12 years ago

Last modified 9 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 12 years ago.
Shot of the actual error
better_operationalerror.diff (782 bytes) - added by jMyles 12 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 12 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 12 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 12 years ago.
Removed psycopg2 import
unwrap_templatesyntaxerror-3.txt (17.5 KB) - added by jMyles 12 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 12 years ago.

Download all attachments as: .zip

Change History (14)

Changed 12 years ago by jMyles

Attachment: Screenshot-1.png added

Shot of the actual error

comment:1 Changed 12 years ago by jMyles

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

Changed 12 years ago by jMyles

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 Changed 12 years ago by jMyles

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.

Changed 12 years ago by jMyles

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 Changed 12 years ago by Carl Meyer

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.

Changed 12 years ago by jMyles

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

Changed 12 years ago by jMyles

Removed psycopg2 import

comment:4 Changed 12 years ago by Carl Meyer

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

Changed 12 years ago by jMyles

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

Changed 12 years ago by jMyles

comment:5 Changed 12 years ago by jMyles

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 Changed 12 years ago by Carl Meyer

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 Changed 9 years ago by Aymeric Augustin

#12995 was a duplicate.

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