Opened 11 years ago

Closed 11 years ago

#21189 closed Bug (fixed)

Correct the usage of bare except clauses in django's source

Reported by: Berker Peksag Owned by: nobody
Component: Core (Other) Version: dev
Severity: Normal Keywords:
Cc: Berker Peksag, bmispelon@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

While I was working on #6834, I found this piece of code in django/template/loader_tags.py:

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:

except (TemplateDoesNotExist, TemplateSyntaxError):

and ran the test suite:

./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!

Change History (2)

comment:1 by Baptiste Mispelon, 11 years ago

Cc: bmispelon@… added
Component: Template system → Core (Other)
Summary: Possible bug because of a bare except in django/template/loader_tags.py → Correct the usage of bare except clauses in django's source
Triage Stage: Unreviewed → Accepted

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:

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

comment:2 by Baptiste Mispelon <bmispelon@…>, 11 years ago

Resolution: → fixed
Status: new → closed

In 20472aa827669d2b83b74e521504e88e18d086a1:

Fixed #21189: Cleaned up usage of bare except clauses.

Thanks to berkerpeksag for the report and to claudep
for the review.

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