Code

Opened 2 years ago

Last modified 14 months ago

#17664 new Bug

Smart `if` tag silencing exceptions plus `QuerySet` caching equals buggy behaviour.

Reported by: mrmachine Owned by: nobody
Component: Template system Version: master
Severity: Normal Keywords: smart if tag queryset exception silenced
Cc: krzysiumed@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Attachments (0)

Change History (10)

comment:1 Changed 2 years ago by akaariai

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

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.

comment:2 Changed 2 years ago by krzysiumed

  • Cc krzysiumed@… added

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?

comment:3 Changed 2 years ago by mrmachine

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.

comment:4 Changed 2 years ago by krzysiumed

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.

comment:5 Changed 2 years ago by aaugustin

  • Triage Stage changed from Unreviewed to Accepted

comment:6 Changed 2 years ago by krzysiumed

  • Component changed from Uncategorized to Template system

comment:7 follow-up: Changed 2 years ago by mrmachine

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?

comment:8 Changed 15 months ago by pjdelport

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.

Last edited 15 months ago by pjdelport (previous) (diff)

comment:9 in reply to: ↑ 7 Changed 15 months ago by pjdelport

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

comment:10 Changed 14 months ago by gnosek

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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as new
The owner will be changed from nobody to anonymous. Next status will be 'assigned'
as The resolution will be set. Next status will be 'closed'
Author


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

 
Note: See TracTickets for help on using tickets.