Code

Opened 3 years ago

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

Download all attachments as: .zip

Change History (13)

Changed 3 years ago by jMyles

Shot of the actual error

comment:1 Changed 3 years ago by jMyles

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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

Changed 3 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 3 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 3 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 3 years ago by carljm

  • Summary changed from TemplateSyntaxError is raised improperly in some cases where database connection fails to Don't wrap exceptions in TemplateSyntaxError under DEBUG
  • Triage Stage changed from Unreviewed to Accepted

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

Removed psycopg2 import

comment:4 Changed 3 years ago by carljm

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

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

Changed 3 years ago by jMyles

comment:5 Changed 3 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 3 years ago by carljm

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

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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.