[Django] #17664: Smart `if` tag silencing exceptions plus `QuerySet` caching equals buggy behaviour.
Django
behaviour.
-------------------------+-------------------------------------------------
Reporter: | Owner: nobody
mrmachine | Status: new
Type: Bug | Version: SVN
Component: | Keywords: smart if tag queryset exception
Uncategorized | silenced
Severity: Normal | Has patch: 0
Triage Stage: | UI/UX: 0
Unreviewed |
Easy pickings: 0 |
-------------------------+-------------------------------------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/17664>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
behaviour.
Reporter: mrmachine | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: SVN
Severity: Normal | Resolution:
Keywords: smart if tag | Triage Stage:
queryset exception silenced | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by akaariai):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/17664#comment:1>
Django
behaviour.
Reporter: mrmachine | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: SVN
Severity: Normal | Resolution:
Keywords: smart if tag | Triage Stage:
queryset exception silenced | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: krzysiumed@… (added)
Comment:
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/17664#comment:2>
Django
behaviour.
Reporter: mrmachine | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: SVN
Severity: Normal | Resolution:
Keywords: smart if tag | Triage Stage:
queryset exception silenced | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/17664#comment:3>
Django
behaviour.
Reporter: mrmachine | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: SVN
Severity: Normal | Resolution:
Keywords: smart if tag | Triage Stage:
queryset exception silenced | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (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`.
--
Ticket URL: <https://code.djangoproject.com/ticket/17664#comment:4>
Django
behaviour.
Reporter: mrmachine | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: SVN
Severity: Normal | Resolution:
queryset exception silenced | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by aaugustin):
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/17664#comment:5>
Django
behaviour.
Reporter: mrmachine | Owner: nobody
Type: Bug | Status: new
Keywords: smart if tag | Triage Stage: Accepted
queryset exception silenced | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* component: Uncategorized => Template system
--
Ticket URL: <https://code.djangoproject.com/ticket/17664#comment:6>
Django
behaviour.
Reporter: mrmachine | Owner: nobody
Type: Bug | Status: new
Component: Template system | Version: SVN
Severity: Normal | Resolution:
Keywords: smart if tag | Triage Stage: Accepted
queryset exception silenced | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (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?
--
Ticket URL: <https://code.djangoproject.com/ticket/17664#comment:7>
Django
behaviour.
Reporter: mrmachine | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: smart if tag | Triage Stage: Accepted
queryset exception silenced | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/17664#comment:8>
Django
behaviour.
Reporter: mrmachine | Owner: nobody
Type: Bug | Status: new
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: smart if tag | Triage Stage: Accepted
queryset exception silenced | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by pjdelport):
This should probably be split into two different tickets, yes.
--
Ticket URL: <https://code.djangoproject.com/ticket/17664#comment:9>
Django
behaviour.
Reporter: mrmachine | Owner: nobody
Type: Bug | Status: new
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: smart if tag | Triage Stage: Accepted
queryset exception silenced | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by gnosek):
I created #19895 to track and fix the queryset iteration issue (which is
separate from {% if not foo %} swallowing exceptions)
--
Ticket URL: <https://code.djangoproject.com/ticket/17664#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: mrmachine | Owner: nobody
Type: Bug | Status: new
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: smart if tag | Triage Stage: Accepted
queryset exception silenced |
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by timgraham):
I confirmed this is still an issue on master. In the case of `{% if not
obj.raises_exception %}`, here is the relevant
[https://github.com/django/django/blob/3862c568ac1b920188ecfbe5cd8073160206d6b9/django/template/smartif.py#L85-L89
exception catching].
If the fix is to remove this exception handling, this will obviously be
backwards incompatible for people relying on silent failure.
--
Ticket URL: <https://code.djangoproject.com/ticket/17664#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: mrmachine | Owner: nobody
Type: Bug | Status: new
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: smart if tag | Triage Stage: Accepted
queryset exception silenced |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by joshmaker):
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
--
Ticket URL: <https://code.djangoproject.com/ticket/17664#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: mrmachine | Owner: nobody
Type: Bug | Status: new
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: smart if tag | Triage Stage: Accepted
queryset exception silenced |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by collinanderson):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/17664#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: mrmachine | Owner: nobody
Type: Bug | Status: new
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: smart if tag | Triage Stage: Accepted
queryset exception silenced |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by joshmaker):
Replying to [comment:13 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/17664#comment:14>
Django
-------------------------------------+-------------------------------------
Reporter: mrmachine | Owner: nobody
Type: Bug | Status: new
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: smart if tag | Triage Stage: Accepted
queryset exception silenced |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by mrmachine):
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...?
--
Ticket URL: <https://code.djangoproject.com/ticket/17664#comment:15>
Django
-------------------------------------+-------------------------------------
Reporter: mrmachine | Owner: nobody
Type: Bug | Status: new
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: smart if tag | Triage Stage: Accepted
queryset exception silenced |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by joshmaker):
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/17664#comment:16>
Django
-------------------------------------+-------------------------------------
Reporter: mrmachine | Owner: nobody
Type: Bug | Status: new
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: smart if tag | Triage Stage: Accepted
queryset exception silenced |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by timgraham):
This ticket came up when discussing #25600 on
[https://groups.google.com/d/topic/django-
developers/BLW5fCTDYHA/discussion django-developers].
--
Ticket URL: <https://code.djangoproject.com/ticket/17664#comment:17>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: smart if tag | Triage Stage: Accepted
queryset exception silenced |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by 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 ?
--
Ticket URL: <https://code.djangoproject.com/ticket/17664#comment:18>
Django
-------------------------------------+-------------------------------------
| Roskam
Type: Bug | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: smart if tag | Triage Stage: Accepted
queryset exception silenced |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => Robert Roskam
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/17664#comment:19>
Django
-------------------------------------+-------------------------------------
Reporter: Tai Lee | Owner: Robert
| Roskam
Type: Bug | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: smart if tag | Triage Stage: Accepted
queryset exception silenced |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Robert Roskam):
I've been able to replicate chambon's additional error report.
--
Ticket URL: <https://code.djangoproject.com/ticket/17664#comment:20>
Django
-------------------------------------+-------------------------------------
Reporter: Tai Lee | Owner: Robert
| Roskam
Type: Bug | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: smart if tag | Triage Stage: Accepted
queryset exception silenced |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/17664#comment:21>
Django
-------------------------------------+-------------------------------------
Reporter: Tai Lee | Owner: Robert
| Roskam
Type: Bug | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: smart if tag | Triage Stage: Accepted
queryset exception silenced |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by 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 that a raises an error and not a
doesn't, 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 assertions 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?
--
Ticket URL: <https://code.djangoproject.com/ticket/17664#comment:22>
Django
-------------------------------------+-------------------------------------
Reporter: Tai Lee | Owner: Robert
| Roskam
Type: Bug | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: smart if tag | Triage Stage: Accepted
queryset exception silenced |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/17664#comment:23>
Django
-------------------------------------+-------------------------------------
Reporter: Tai Lee | Owner: Robert
| Roskam
Type: Bug | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: smart if tag | Triage Stage: Accepted
queryset exception silenced |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/17664#comment:24>
Django
-------------------------------------+-------------------------------------
Reporter: Tai Lee | Owner: Robert
| Roskam
Type: Bug | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: smart if tag | Triage Stage: Accepted
queryset exception silenced |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
Comment:
https://github.com/django/django/pull/12752
--
Ticket URL: <https://code.djangoproject.com/ticket/17664#comment:25>
Django
-------------------------------------+-------------------------------------
Reporter: Tai Lee | Owner: Robert
| Roskam
Type: Bug | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: smart if tag | Triage Stage: Accepted
queryset exception silenced |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/17664#comment:26>
Django
-------------------------------------+-------------------------------------
Reporter: Tai Lee | Owner: Robert
| Roskam
Type: Bug | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: smart if tag | Triage Stage: Accepted
queryset exception silenced |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/17664#comment:27>
Django
-------------------------------------+-------------------------------------
Reporter: Tai Lee | Owner: Robert
| Roskam
Type: Bug | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: smart if tag | Triage Stage: Accepted
queryset exception silenced |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/17664#comment:28>
Django
-------------------------------------+-------------------------------------
Reporter: Tai Lee | Owner: Robert
| Roskam
Type: Bug | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: smart if tag | Triage Stage: Accepted
queryset exception silenced |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by felixxm):
I totally agree with
[https://code.djangoproject.com/ticket/17664#comment:27 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
([https://code.djangoproject.com/ticket/17664#comment:13 as suggested by
Colin]). `AttributeError`, `KeyError`, and `IndexError` sound like a good
starting point.
--
Ticket URL: <https://code.djangoproject.com/ticket/17664#comment:29>
Django
-------------------------------------+-------------------------------------
Reporter: Tai Lee | Owner: Robert
| Roskam
Type: Bug | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: smart if tag | Triage Stage: Accepted
queryset exception silenced |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 1 => 0
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/17664#comment:30>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: smart if tag | Triage Stage: Accepted
queryset exception silenced |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Carlton Gibson):
* owner: Robert Roskam => (none)
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/17664#comment:31>
Django
-------------------------------------+-------------------------------------
Reporter: Tai Lee | Owner: (none)
Type: Bug | Status: new
Keywords: smart if tag | Triage Stage: Accepted
queryset exception silenced |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Ülgen Sarıkavak (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/17664#comment:32>