{% if %} template tag silences exceptions inconsistently

[Tai Lee]

This buggy behaviour took me a while to track down. I hope I can explain it clearly.

Given a QuerySet with invalid ordering (and possibly other conditions) that should raise an exception when evaluated, {% if qs %} will raise the exception while {% if not qs %} will silence it and leave qs as if it were simply empty on subsequent access in the template.

First, the exception is silenced and the queryset becomes empty:

>>> from django.contrib.auth.models import User
>>> from django.template import Template, Context
>>> Template('count: {{ qs.count }}, empty: {% if not qs %}yes{% else %}no{% endif %}, qs: {{ qs }}, count: {{ qs.count }}').render(Context({'qs': User.objects.order_by('invalid_field')}))
u'count: 98, empty: no, qs: [], count: 0'

And now if we swap the {% if %} around a bit, we get an exception:

>>> Template('count: {{ qs.count }}, empty: {% if qs %}no{% else %}yes{% endif %}, qs: {{ qs }}, count: {{ qs.count }}').render(Context({'qs': User.objects.order_by('invalid_field')}))
Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "/Users/mrmachine/django/template/base.py", line 139, in render
    return self._render(context)
  File "/Users/mrmachine/django/template/base.py", line 133, in _render
    return self.nodelist.render(context)
  File "/Users/mrmachine/django/template/base.py", line 819, in render
    bits = []
  File "/Users/mrmachine/django/template/debug.py", line 73, in render_node
    return node.render(context)
  File "/Users/mrmachine/django/template/defaulttags.py", line 273, in render
    if var:
  File "/Users/mrmachine/django/db/models/query.py", line 129, in __nonzero__
    iter(self).next()
  File "/Users/mrmachine/django/db/models/query.py", line 117, in _result_iter
    self._fill_cache()
  File "/Users/mrmachine/django/db/models/query.py", line 855, in _fill_cache
    self._result_cache.append(self._iter.next())
  File "/Users/mrmachine/django/db/models/query.py", line 288, in iterator
    for row in compiler.results_iter():
  File "/Users/mrmachine/django/db/models/sql/compiler.py", line 704, in results_iter
    for rows in self.execute_sql(MULTI):
  File "/Users/mrmachine/django/db/models/sql/compiler.py", line 750, in execute_sql
    sql, params = self.as_sql()
  File "/Users/mrmachine/django/db/models/sql/compiler.py", line 64, in as_sql
    ordering, ordering_group_by = self.get_ordering()
  File "/Users/mrmachine/django/db/models/sql/compiler.py", line 364, in get_ordering
    self.query.model._meta, default_order=asc):
  File "/Users/mrmachine/django/db/models/sql/compiler.py", line 393, in find_ordering_name
    opts, alias, False)
  File "/Users/mrmachine/django/db/models/sql/query.py", line 1289, in setup_joins
    "Choices are: %s" % (name, ", ".join(names)))
FieldError: Cannot resolve keyword 'invalid_field' into field. Choices are: date_joined, email, first_name, groups, id, is_active, is_staff, is_superuser, last_login, last_name, logentry, password, user_permissions, username

I think the {% if %} tag needs to be a little less quick to silence exceptions here, but there is also a problem with the way a QuerySet will appear to be empty after an exception is raised.

First, we get an exception:

>>> qs = User.objects.order_by('abc')
>>> list(qs)
Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "/Users/mrmachine/django/db/models/query.py", line 86, in __len__
    self._result_cache.extend(self._iter)
  File "/Users/mrmachine/django/db/models/query.py", line 288, in iterator
    for row in compiler.results_iter():
  File "/Users/mrmachine/django/db/models/sql/compiler.py", line 704, in results_iter
    for rows in self.execute_sql(MULTI):
  File "/Users/mrmachine/django/db/models/sql/compiler.py", line 750, in execute_sql
    sql, params = self.as_sql()
  File "/Users/mrmachine/django/db/models/sql/compiler.py", line 64, in as_sql
    ordering, ordering_group_by = self.get_ordering()
  File "/Users/mrmachine/django/db/models/sql/compiler.py", line 364, in get_ordering
    self.query.model._meta, default_order=asc):
  File "/Users/mrmachine/django/db/models/sql/compiler.py", line 393, in find_ordering_name
    opts, alias, False)
  File "/Users/mrmachine/django/db/models/sql/query.py", line 1289, in setup_joins
    "Choices are: %s" % (name, ", ".join(names)))
FieldError: Cannot resolve keyword 'abc' into field. Choices are: date_joined, email, first_name, groups, id, is_active, is_staff, is_superuser, last_login, last_name, logentry, password, user_permissions, username

Then, we appear to have an empty queryset:

>>> list(qs)
[]

Even though the Query is still invalid:

>>> print qs.query
Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "/Users/mrmachine/django/db/models/sql/query.py", line 167, in __str__
    sql, params = self.sql_with_params()
  File "/Users/mrmachine/django/db/models/sql/query.py", line 175, in sql_with_params
    return self.get_compiler(DEFAULT_DB_ALIAS).as_sql()
  File "/Users/mrmachine/django/db/models/sql/compiler.py", line 64, in as_sql
    ordering, ordering_group_by = self.get_ordering()
  File "/Users/mrmachine/django/db/models/sql/compiler.py", line 364, in get_ordering
    self.query.model._meta, default_order=asc):
  File "/Users/mrmachine/django/db/models/sql/compiler.py", line 393, in find_ordering_name
    opts, alias, False)
  File "/Users/mrmachine/django/db/models/sql/query.py", line 1289, in setup_joins
    "Choices are: %s" % (name, ", ".join(names)))
FieldError: Cannot resolve keyword 'abc' into field. Choices are: date_joined, email, first_name, groups, id, is_active, is_staff, is_superuser, last_login, last_name, logentry, password, user_permissions, username

I think that this only becomes a problem when the exception is mistakenly silenced or not handled correctly, as in the example with the {% if %} tag. In most circumstances, the exception would be raised and further processing would not occur.

However, I think we should still look at the possibility of NOT caching results when a queryset fails to evaluate in this way. I do not think it is appropriate to equate an invalid queryset with an empty one. If an exception is raised once when trying to evaluate a queryset, the exception should be cached or the queryset should be re-evaluated when it is accessed again.

[Anssi Kääriäinen]

This silencing of exceptions raised from compiler constructing the SQL is really annoying when hacking the ORM. I assume there is some good technical reason for this (like this being a problem of Python's generators) but haven't really looked. A big +10 from me to fixing this behavior if that is possible.

[Christopher Medrela]

The problem is that QuerySets are lazy and the exception occures in different places in these two situations.

Look here: https://code.djangoproject.com/browser/django/trunk/django/template/defaulttags.py#L269.

Consider the situation with {% if qs %}. When the <IfNode> is being rendering, attribute condition is TemplateLiteral instance with text="qs". After executing line 274, attribute match is QuerySet. There is no exception, because QuerySets are lazy. Then line 280 becomes. Method __nonzero__ of QuerySet is being executing and the exception is being raising at this moment.

Now consider {% if not qs %}. Line 274 is being executing. What happened? Operator not is being executing. See https://code.djangoproject.com/browser/django/trunk/django/template/smartif.py#L81 - all exceptions are caught. match is False.

The solution may be to change https://code.djangoproject.com/browser/django/trunk/django/template/smartif.py#L84 - except clause should be less strict and should not catch all exception. But which ones?

[Tai Lee]

I'm not sure I really follow the inner workings of the smart if tag. But why would qs from {% if qs %} still be lazy (not evaluated) when {% if not qs %} is no longer lazy (is evaluated)? I thought the first would be equivalent to if qs in Python, which is if bool(qs)? Or is {% if qs %} just testing that the variable exists, and doesn't check the value of it?

Either way, what do you think about the way QuerySet will be an empty queryset after an exception is raised, even though the query is still invalid and never executed? Shouldn't QuerySet cache the exception (not an empty result set), or just not cache anything and re-execute the query when evaluated a second time? This wouldn't fix the {% if qs %} / {% if not qs %} disparity, but it would prevent the strange side-effect where subsequent attempts to access qs yield an empty queryset.

[Christopher Medrela]

mrmachine, look at example:

>>> invalid_qs = Book.objects.order_by('invalid_field') # no error occures
>>> type(invalid_qs)
<class 'django.db.models.query.QuerySet'>
>>> bool(invalid_qs) # this will raise exception
Traceback (most recent call last):
(... traceback ...)
FieldError: Cannot resolve keyword 'invalid_field' into field. Choices are: id, membership, name, part

The queryset is evaluated in both situations -- {% if qs %} as well as {% if not qs %}, but it is evaluated in different places in python code.

Queryset from {% if qs %} is evaluated here: https://code.djangoproject.com/browser/django/trunk/django/template/defaulttags.py#L280 and this line raises our FieldError. There is no try-except so the exception is not caught and we can see traceback.

Queryset from {% if not qs %} is evaluated here: https://code.djangoproject.com/browser/django/trunk/django/template/smartif.py#L98 (and line 83). Line 83:

return func(context, self.first)

calls lambda function from line 98:

lambda context, x: not x.eval(context))

x.eval(context) is our QuerySet. Next, we want to negate the queryset because of not operator, so bool(queryset) is computed and it raises exception. We back to line 83 of smartif.py, where the lambda-function was called. As you can see, there is except clause and it catches every exception including our FieldError.

[Tai Lee]

This is also an ORM (queryset caching) issue. Not sure if this should be split into two different tickets, or if the fix should be in the template or in the ORM?

[Pi Delport]

I ran into this template system problem, independent of querysets, with a filter that triggered an AssertionError.

The following example raises the error:

{% if var|some_filter %}yes{% else %}no{% endif %}

while the following examples both swallow the error, and (surprisingly!) evaluate to "no":

{% if not var|somefilter %}yes{% else %}no{% endif %}
{% if True or var|somefilter %}yes{% else %}no{% endif %}

krzysiumed's analysis of the template evaluation behavior is spot-on.

I believe the current behavior is broken, regardless of the question of whether templates should raise exceptions or not. If an exception-raising filter like var|somefilter is supposed to evaluate to the empty string, then one would in turn expect expressions like not var|somefilter and True or var|somefilter to interpret the empty string and evaluate to true (or whatever is appropriate), instead of effectively ignoring the intermediate expressions and always evaluating to false, as above.

[Pi Delport]

This should probably be split into two different tickets, yes.

[Grzegorz Nosek]

I created #19895 to track and fix the queryset iteration issue (which is separate from {% if not foo %} swallowing exceptions)

[Tim Graham]

I confirmed this is still an issue on master. In the case of {% if not obj.raises_exception %}, here is the relevant ​exception catching.

If the fix is to remove this exception handling, this will obviously be backwards incompatible for people relying on silent failure.

[Josh West]

I think that the behavior most developers would expect from the {% if ... %} template tag is for it to handle exceptions similar to how they are handled in template variable resolution (see: django.template.base.Variable). In other words, AttributeError, KeyError, ValueError, and IndexError as well as exceptions where silent_variable_failure == True would be silenced and False returned, but all other exceptions would be raised.

Here is one proposed solution:
​https://github.com/django/django/compare/master...theatlantic:ticket_17664

[Collin Anderson]

Silencing AttributeError, KeyError, IndexError makes sense to me. Why silence ValueError and TypeError? Is that for filters? I suppose the more exceptions we catch the better backwards compatibility we get.

[Josh West]

Replying to collinanderson:

Why silence ValueError and TypeError? Is that for filters?

My thought process on this was basically limited to matching the existing list of Exceptions listed out here: ​https://github.com/django/django/blob/1.8.4/django/template/base.py#L833-L836 so the behavior would match that for template variable resolution.

[Tai Lee]

If we are going to silence exceptions the same way as template variable resolution, do we need special handling for filters that may be applied to the variable after it is resolved?

The docs say:

Since the template language doesn’t provide exception handling, any exception raised from a template filter will be exposed as a server error. Thus, filter functions should avoid raising exceptions if there is a reasonable fallback value to return. In case of input that represents a clear bug in a template, raising an exception may still be better than silent failure which hides the bug.

So for the following code:

{% if not foo.bar|filter_that_raises_AttributeError %}

I would expect to see a 500 response.

But for:

{% if not foo.bar_raises_AttributeError %}

I would not...?

[Josh West]

As a user, my expectation would be that the behavior of:
{% if not foo.bar|filter_that_raises_AttributeError %}

would be identical to:
{{ foo.bar|filter_that_raises_AttributeError }}

with regards to the exceptions raised. Right?

[Tim Graham]

This ticket came up when discussing #25600 on ​django-developers.

[pascal chambon]

Apart from the "string_if_invalid" issue, {% if %} tags also seem to silence nasty exceptions (RuntimeError etc.) as soon as there are operators, under Django==1.10.5 :

{% if participant_challenge.list.0.role_participant %} --> raises server error (due to bug in role_participant())

{% if participant_challenge.list.0.role_participant == 1 %} --> does not raise anything

Is it an expected behaviour ?

[Robert Roskam]

I've been able to replicate chambon's additional error report. The key difference here can be simplified this way:

Case 1

{% if a.b %}

Case 2

{% if a.b == 1 %}

where b is a callable raising an error.

My reading so far feels incomplete in this area, but the way the parser seems to work is that if there is only 1 token to evaluate. It skips all the logic to eat the error. However, if it has multiple tokens, it hits all the appropriate logic.

This happens because there's no operator for it to match (​https://github.com/django/django/blob/master/django/template/smartif.py#L174)

[Robert Roskam]

So I've figured out what's going on here. We have 2 distant parts interacting here:

(1) the templatetag if parser (​​https://github.com/django/django/blob/master/django/template/smartif.py#L174)
(2) the lookup_resolver (​https://github.com/django/django/blob/master/django/template/base.py#L819)

Template literals with no booleans operations are passed down through the if parser in (1) and no evaluation is down on them. All the capturing of values and forcing them to False is done in the infix and prefix functions of (1). Since both of these 2 previous things are true, any exception that is raised will be caught if it uses any boolean logic, so given that:

def _raise_exception():
    raise Exception
a = _raise_exception

All of these will raises exceptions:

{% if a %}

{% if a|length %}

{% if a||add:"2" %}

However, all of these will coerce a to False:

{% if a == 1 %}

{% if a and a%}

{% if not not a %} 

As best as I can tell, these are being totally consistent with their design intent. That is to say, they're operating exactly like the docs say they should. If we don't like the fact that a raises an error and not a doesn't, then I think we should make a deliberate design decision to do one or the other.

[Robert Roskam]

So I realize that I may have buried the lead in my previous post:

"As best as I can tell, these are being totally consistent with their design intent. That is to say, they're operating exactly like the docs say they should. If we don't like the fact.., then I think we should make a deliberate design decision."

And then I gave 2 options, but I'm going to just focus on one of them: I think the choice to let boolean operations silence exceptions was a mistake. I think we should change this.

We can't change the interface and offer a different "if" statement, so this is a breaking change, since some projects may have been relying on this behavior. So the intermediate step might be to raise a warning in the console for a minor version that exceptions will stopped being coerced to False soon.

Thoughts?

[Jon Dufresne]

I think the choice to let boolean operations silence exceptions was a mistake. I think we should change this.

I agree. Boolean conditions often hide private data. In the case of a programming mistake, I'd prefer an exception thrown so the programmer can fix it instead of plowing ahead with a potentially incorrect condition. Depending on the template, assuming a value could display private data to the wrong user.

So the intermediate step might be to raise a warning in the console for a minor version that exceptions will stopped being coerced to False soon.

+1 This approach sounds sensible to me.

[Robert Roskam]

OK, I think I've let this sit out there for a good long time now. I haven't seen anyone disagree and 1 in favor. So I'm going to treat that as a soft acceptance. I'll be working on a patch to add a warning to this change in behavior.

[Jon Dufresne]

​https://github.com/django/django/pull/12752

[Carlton Gibson]

So current PR removes the ability to silently move over undefined variables. As such it requires changes to check if a variable is defined before accessing it in the template:

{% if request and model.admin_url and model.admin_url in request.path %}

Where the {% if request and model.admin_url and ... is new.

For me this would be a major change to the (historic) behaviour of the DTL, and a major step back in usability. It's an age old feature to be able to define blocks that simple don't show if the variable is missing. (The number of place is used must be legion.)

Beyond that the if var and var.attr pattern is horribly boilerplate-y. (IMO)

Is there a way we can allow genuine exceptions to raise whilst maintaining the behaviour of allowing variables to be undefined, passing only on a missing lookup, say? What would that look like?

I think we'd need significantly more light on the discussion before making the suggested change: we need to be sure we've realised what the change really entails.
Hope that makes sense.

[Jon Dufresne]

Added details in the PR that explains why the exceptions must be raised to avoid typos or other programming mistakes from inadvertently exposing private or sensitive data. As {% if %} template tags are used to guard private details, ignoring undefined variables (possibly) due to a typo, can result in easily -- though mistakenly -- exposing this sensitive data.

[Mariusz Felisiak]

I totally agree with Carlton. This is a huge change in the current longstanding behavior that would make the DTL less user-friendly and would force users to rewrite templates in bulk with no benefits. I also don't agree that the current behavior has important security implications, even if it's used for permission checks then showing sensitive data in {% else %} block seems like an unusual pattern, moreover to expose sensitive data we need to also make a typo and don't have any tests, that's an edge case, which should not force most of users to rewrite their templates. In order to work with the DTL's behavior, moving such logic up to the view or into a template tag are the long established approaches.

I think we should decide which exceptions should be suppressed (as suggested by Colin). AttributeError, KeyError, and IndexError sound like a good starting point.

[Carlton Gibson]

OK, I finally got the time to get back to this. Reviewing again, I still can't see that we can introduce the radical breaking change. Limited raising of exceptions that don't reasonably pertain to variable resolution seems as far as we can go. I'll close the suggested PR on that basis.