[Django] #21189: Possible bug because of a bare except in django/template/loader

Django

unread,

Sep 28, 2013, 3:30:49 PM9/28/13

to django-...@googlegroups.com

#21189: Possible bug because of a bare except in django/template/loader_tags.py
---------------------------------+--------------------
Reporter: berkerpeksag | Owner: nobody
Type: Bug | Status: new
Component: Template system | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+--------------------
While I was working on #6834, I found this piece of code in
django/template/loader_tags.py:

{{{#!python

def render(self, context):
try:
template = self.template.resolve(context)
# Does this quack like a Template?
if not callable(getattr(template, 'render', None)):
# If not, we'll try get_template
template = get_template(template)
values = {
name: var.resolve(context)
for name, var in six.iteritems(self.extra_context)
}
if self.isolated_context:
return template.render(context.new(values))
with context.push(**values):
return template.render(context)
except:
if settings.TEMPLATE_DEBUG:
raise
return ''
}}}

See from GitHub:
https://github.com/django/django/blob/master/django/template/loader_tags.py#L146

I change the bare except in line 146 to:

{{{#!python
except (TemplateDoesNotExist, TemplateSyntaxError):
}}}

and ran the test suite:

{{{#!sh
./runtests.py --settings=test_sqlite template_tests
}}}

Here is the output:
https://gist.github.com/berkerpeksag/ab84b067a552951abd9c

So, is the usage of bare except correct?

Thanks!

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

Django

unread,

Sep 29, 2013, 5:46:34 PM9/29/13

to django-...@googlegroups.com

#21189: Correct the usage of bare except clauses in django's source
------------------------------+------------------------------------

Reporter: berkerpeksag | Owner: nobody
Type: Bug | Status: new

Easy pickings: 0 | UI/UX: 0

------------------------------+------------------------------------
Changes (by bmispelon):

* cc: bmispelon@… (added)
* needs_better_patch: => 0
* component: Template system => Core (Other)
* needs_tests: => 0
* needs_docs: => 0
* stage: Unreviewed => Accepted

Comment:

Hi,

The idea behind the bare `except` is that rendering should catch all
exceptions and, depending on the value of `settings.TEMPLATE_DEBUG`,
either output an empty string or re-raise it (it will be caught later down
the chain to display the familiar error page).

This is by design and it explains the test failures you get when you
restrict the types of exceptions being caught.

With that said, the usage of a bare `except` clause is usually a code
smell (and pep8 advises against it [1]). In this case for example, `except
Exception` might be better suited (`except Exception` catches all
exceptions except those like `SystemExit` which you generally don't want
to catch).

I did an audit of django's code to find the other instances of using bare
except clauses and I think there's room for improvement so I'll mark this
ticket as `accepted` but broaden its scope to cover other bare except
clauses.

The difficulty here lies in finding out which exception type(s) is
appropriate to catch (in general, while using `except Exception` is an
improvement over `except`, it's still too broad in most cases). This has
to be done on a case-by-case basis and usually require a good
understanding of the surrounding code.

So here's the list of places where I think `except` clauses should be
qualified.
Note that the line numbers are likely to get out of sync as time passes
(as I'm typing this, the current HEAD on master is at
c4fdd859ecba0b8e6dac6eca06429e66925722bd).
You can use `grep -rn except: django/` to find the occurences in your own
checkout.

There's also two instances where I'm not sure whether a bare `except` is
appropriate or not:

*
[https://github.com/django/django/blob/master/django/utils/module_loading.py#L60
django/utils/module_loading.py:60]
*
[https://github.com/django/django/blob/master/django/contrib/flatpages/middleware.py#L15
django/contrib/flatpages/middleware.py:15]

Finally, for completeness's sake, here are the instances of bare `except`
clauses which I believe are correct:

Of course, this doesn't have to be tackled all at once, so if anyone wants
to offer a patch for only one or two items in the list, I'd be happy to
review it (you also get bonus point if you can provided a testcase that
illustrates incorrect silencing of a particular type of exception).

Thanks for the report (and sorry for the big wall of text :) ).

[1] http://www.python.org/dev/peps/pep-0008/#programming-recommendations

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

Django

unread,

Oct 5, 2013, 6:42:00 AM10/5/13

to django-...@googlegroups.com

#21189: Correct the usage of bare except clauses in django's source
------------------------------+------------------------------------

Reporter: berkerpeksag | Owner: nobody
Type: Bug | Status: closed

Component: Core (Other) | Version: master

Severity: Normal | Resolution: fixed

Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0

------------------------------+------------------------------------
Changes (by Baptiste Mispelon <bmispelon@…>):

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

Comment:

In [changeset:"20472aa827669d2b83b74e521504e88e18d086a1"]:
{{{
#!CommitTicketReference repository=""
revision="20472aa827669d2b83b74e521504e88e18d086a1"
Fixed #21189: Cleaned up usage of bare except clauses.

[Django] #21189: Possible bug because of a bare except in django/template/loader_tags.py

Django

Django

Django