Opened 13 years ago
Last modified 9 months ago
#17664 new Bug
{% if %} template tag silences exceptions inconsistently
Reported by: | Tai Lee | Owned by: | |
---|---|---|---|
Component: | Template system | Version: | dev |
Severity: | Normal | Keywords: | smart if tag queryset exception silenced |
Cc: | krzysiumed@…, Ülgen Sarıkavak | 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.
Change History (32)
comment:1 by , 13 years ago
comment:2 by , 13 years ago
Cc: | 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 by , 13 years ago
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 by , 13 years ago
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 by , 13 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:6 by , 13 years ago
Component: | Uncategorized → Template system |
---|
follow-up: 9 comment:7 by , 13 years ago
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 by , 12 years ago
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.
comment:10 by , 12 years ago
I created #19895 to track and fix the queryset iteration issue (which is separate from {% if not foo %} swallowing exceptions)
comment:11 by , 9 years ago
Summary: | Smart `if` tag silencing exceptions plus `QuerySet` caching equals buggy behaviour. → Smart `if` tag silences exceptions |
---|
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.
comment:12 by , 9 years ago
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
follow-up: 14 comment:13 by , 9 years ago
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.
comment:14 by , 9 years ago
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.
comment:15 by , 9 years ago
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...?
comment:16 by , 9 years ago
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?
comment:17 by , 9 years ago
Summary: | Smart `if` tag silences exceptions → {% if %} template tag silences exceptions inconsistently |
---|
This ticket came up when discussing #25600 on django-developers.
comment:18 by , 7 years ago
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 ?
comment:19 by , 7 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:20 by , 7 years ago
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.
comment:21 by , 7 years ago
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 not a %} {% if a == 1 %} {% if a and a%}
Therefore, if you combine them, they get silenced:
{% if not a|length %} {% if a|length == 1 %} {% if a and a|length%}
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 either(1) start letting all the boolean comparisons no longer just give False up to the template OR (2) we figure out when we don't mind silencing filters in if tags.
comment:22 by , 7 years ago
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?
comment:23 by , 7 years ago
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.
comment:24 by , 7 years ago
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.
comment:26 by , 4 years ago
Patch needs improvement: | set |
---|
comment:27 by , 4 years ago
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.
comment:28 by , 4 years ago
Patch needs improvement: | unset |
---|
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.
comment:29 by , 4 years ago
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.
comment:30 by , 4 years ago
Has patch: | unset |
---|
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.
comment:31 by , 4 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
comment:32 by , 9 months ago
Cc: | added |
---|
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.