Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed #24977 -- Made template tags treat undefined variables not equal to None. #7901

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions AUTHORS
Expand Up @@ -751,6 +751,7 @@ answer newbie questions, and generally made Django that much better:
tibimicu@gmx.net
Tim Graham <timograham@gmail.com>
Tim Heap <tim@timheap.me>
Tim Martin <tim@asymptotic.co.uk>
Tim Saylor <tim.saylor@gmail.com>
Tobias Kunze <rixx@cutebit.de>
Tobias McNulty <http://www.caktusgroup.com/blog>
Expand Down
45 changes: 34 additions & 11 deletions django/template/base.py
Expand Up @@ -117,6 +117,32 @@ def __str__(self):
return self.msg % self.params


class UndefinedVariable:
"""
This is the result of evaluating a template expression that
contains an undefined variable.

An instance of this class evaluates to False in a boolean
context. All instances are treated as being equal to each other.
"""

def __init__(self, var_rep=""):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think var_repr is more explanatory than just rep. Also, use single quotes rather than double quotes unless the string contains a single quote.

"""
`var_rep` is the string value that this expression should
expand to when displaying in a template.
"""
self.var_rep = var_rep

def __bool__(self):
return False

def __eq__(self, other):
return isinstance(other, UndefinedVariable)

def __str__(self):
return self.var_rep


class Origin:
def __init__(self, name, template_name=None, loader=None):
self.name = name
Expand Down Expand Up @@ -672,22 +698,19 @@ def __init__(self, token, parser):
self.filters = filters
self.var = var_obj

def resolve(self, context, ignore_failures=False):
def resolve(self, context):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess ignore_failures should go through a deprecation path?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was assuming this wasn't necessary since FilterExpression isn't part of the public API, and all the internal usages have been updated.

if isinstance(self.var, Variable):
try:
obj = self.var.resolve(context)
except VariableDoesNotExist:
if ignore_failures:
obj = None
else:
string_if_invalid = context.template.engine.string_if_invalid
if string_if_invalid:
if '%s' in string_if_invalid:
return string_if_invalid % self.var
else:
return string_if_invalid
string_if_invalid = context.template.engine.string_if_invalid
if string_if_invalid:
if '%s' in string_if_invalid:
obj = UndefinedVariable(string_if_invalid % self.var)
else:
obj = string_if_invalid
obj = UndefinedVariable(string_if_invalid)
else:
obj = UndefinedVariable()
else:
obj = self.var
for func, args in self.filters:
Expand Down
26 changes: 14 additions & 12 deletions django/template/defaulttags.py
Expand Up @@ -16,8 +16,8 @@
BLOCK_TAG_END, BLOCK_TAG_START, COMMENT_TAG_END, COMMENT_TAG_START,
FILTER_SEPARATOR, SINGLE_BRACE_END, SINGLE_BRACE_START,
VARIABLE_ATTRIBUTE_SEPARATOR, VARIABLE_TAG_END, VARIABLE_TAG_START,
Context, Node, NodeList, TemplateSyntaxError, VariableDoesNotExist,
kwarg_re, render_value_in_context, token_kwargs,
Context, Node, NodeList, TemplateSyntaxError, UndefinedVariable,
VariableDoesNotExist, kwarg_re, render_value_in_context, token_kwargs,
)
from .defaultfilters import date
from .library import Library
Expand Down Expand Up @@ -119,7 +119,7 @@ def __init__(self, variables, asvar=None):

def render(self, context):
for var in self.vars:
value = var.resolve(context, True)
value = var.resolve(context)
if value:
first = render_value_in_context(value, context)
if self.asvar:
Expand Down Expand Up @@ -160,10 +160,10 @@ def render(self, context):
parentloop = {}
with context.push():
try:
values = self.sequence.resolve(context, True)
values = self.sequence.resolve(context)
except VariableDoesNotExist:
values = []
if values is None:
if isinstance(values, UndefinedVariable):
values = []
if not hasattr(values, '__len__'):
values = list(values)
Expand Down Expand Up @@ -240,7 +240,7 @@ def render(self, context):
if self._varlist:
# Consider multiple parameters. This automatically behaves
# like an OR evaluation of the multiple variables.
compare_to = [var.resolve(context, True) for var in self._varlist]
compare_to = [var.resolve(context) for var in self._varlist]
else:
# The "{% ifchanged %}" syntax (without any variables) compares the rendered output.
compare_to = nodelist_true_output = self.nodelist_true.render(context)
Expand Down Expand Up @@ -280,8 +280,8 @@ def __repr__(self):
return '<%s>' % self.__class__.__name__

def render(self, context):
val1 = self.var1.resolve(context, True)
val2 = self.var2.resolve(context, True)
val1 = self.var1.resolve(context)
val2 = self.var2.resolve(context)
if (self.negate and val1 != val2) or (not self.negate and val1 == val2):
return self.nodelist_true.render(context)
return self.nodelist_false.render(context)
Expand Down Expand Up @@ -351,11 +351,11 @@ def resolve_expression(self, obj, context):
# This method is called for each object in self.target. See regroup()
# for the reason why we temporarily put the object in the context.
context[self.var_name] = obj
return self.expression.resolve(context, True)
return self.expression.resolve(context)

def render(self, context):
obj_list = self.target.resolve(context, True)
if obj_list is None:
obj_list = self.target.resolve(context)
if isinstance(obj_list, UndefinedVariable):
# target variable wasn't found in context; fail silently.
context[self.var_name] = []
return ''
Expand Down Expand Up @@ -438,6 +438,8 @@ def render(self, context):
args = [arg.resolve(context) for arg in self.args]
kwargs = {k: v.resolve(context) for k, v in self.kwargs.items()}
view_name = self.view_name.resolve(context)
if isinstance(view_name, UndefinedVariable):
raise TemplateSyntaxError("Variable %s is not defined" % self.view_name)
try:
current_app = context.request.current_app
except AttributeError:
Expand Down Expand Up @@ -882,7 +884,7 @@ def display(self):
return self.text

def eval(self, context):
return self.value.resolve(context, ignore_failures=True)
return self.value.resolve(context)


class TemplateIfParser(IfParser):
Expand Down
8 changes: 5 additions & 3 deletions django/template/loader_tags.py
Expand Up @@ -7,7 +7,8 @@
from django.utils.safestring import mark_safe

from .base import (
Node, Template, TemplateSyntaxError, TextNode, Variable, token_kwargs,
Node, Template, TemplateSyntaxError, TextNode, UndefinedVariable, Variable,
token_kwargs,
)
from .library import Library

Expand Down Expand Up @@ -113,8 +114,9 @@ def find_template(self, template_name, context):

def get_parent(self, context):
parent = self.parent_name.resolve(context)
if not parent:
error_msg = "Invalid template name in 'extends' tag: %r." % parent
if isinstance(parent, UndefinedVariable):
error_msg = "Undefined variable in 'extends' tag."

if self.parent_name.filters or\
isinstance(self.parent_name.var, Variable):
error_msg += " Got this from the '%s' variable." %\
Expand Down
19 changes: 19 additions & 0 deletions docs/releases/2.0.txt
Expand Up @@ -271,6 +271,25 @@ If you wish to keep this restriction in the admin when editing users, set
admin.site.unregister(User)
admin.site.register(User, MyUserAdmin)

Undefined variables in templates are distinguished from `None`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

double backticks must be used for correct formatting of the rendered docs

--------------------------------------------------------------

In templates, undefined values and non-existent attributes of objects
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nonexistent (no dash)

are now treated as being instances of a special type that don't
compare equal to any other Python object (including `None`). In older
versions of Django, such a variable would be treated as having a value
of `None`, meaning that comparisons could succeed unexpectedly due to
a typo or refactoring::

{% if user.pk == product.product_owner_id %}
If the product_owner_id field is renamed, this private data would
have been rendered for users who are not logged in.
{% endif %}

Such code will now fail the comparison if one side contains an
undefined value. You should avoid deliberately relying on undefined
values in your templates.

Miscellaneous
-------------

Expand Down
6 changes: 6 additions & 0 deletions tests/template_tests/filter_tests/test_length.py
Expand Up @@ -43,6 +43,12 @@ def test_length07(self):
output = self.engine.render_to_string('length07', {'None': None})
self.assertEqual(output, '0')

@setup({'length08': '{{ missing_var|length }}'})
def test_length08(self):
"""Missing variables have a length of zero."""
output = self.engine.render_to_string('length08', {})
self.assertEqual(output, '0')


class FunctionTests(SimpleTestCase):

Expand Down
9 changes: 3 additions & 6 deletions tests/template_tests/syntax_tests/test_exceptions.py
Expand Up @@ -20,12 +20,9 @@ def test_exception02(self):
"""
Raise exception for invalid variable template name
"""
if self.engine.string_if_invalid:
with self.assertRaises(TemplateDoesNotExist):
self.engine.render_to_string('exception02')
else:
with self.assertRaises(TemplateSyntaxError):
self.engine.render_to_string('exception02')
with self.assertRaisesRegexp(TemplateSyntaxError,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use:

msg = "Got this from the 'nonexistent' variable"
with self.assertRaisesMessage(TemplateSyntaxError, msg):

`assertRraisesMessage

"Got this from the 'nonexistent' variable"):
self.engine.render_to_string('exception02')

@setup(
{'exception03': "{% extends 'inheritance01' %}"
Expand Down
16 changes: 13 additions & 3 deletions tests/template_tests/syntax_tests/test_if.py
Expand Up @@ -543,10 +543,12 @@ def test_if_tag_badarg02(self):
output = self.engine.render_to_string('if-tag-badarg02', {'y': 0})
self.assertEqual(output, '')

@setup({'if-tag-badarg03': '{% if x|default_if_none:y %}yes{% endif %}'})
@setup({'if-tag-badarg03': '{% if x|default_if_none:y %}yes{% else %}no{% endif %}'})
def test_if_tag_badarg03(self):
# default_if_none is not triggered since undefined variables
# are not None.
output = self.engine.render_to_string('if-tag-badarg03', {'y': 1})
self.assertEqual(output, 'yes')
self.assertEqual(output, 'no')

@setup({'if-tag-badarg04': '{% if x|default_if_none:y %}yes{% else %}no{% endif %}'})
def test_if_tag_badarg04(self):
Expand Down Expand Up @@ -577,7 +579,7 @@ def test_if_is_variable_missing(self):
@setup({'template': '{% if foo is bar %}yes{% else %}no{% endif %}'})
def test_if_is_both_variables_missing(self):
output = self.engine.render_to_string('template', {})
self.assertEqual(output, 'yes')
self.assertEqual(output, 'no')

@setup({'template': '{% if foo is not None %}yes{% else %}no{% endif %}'})
def test_if_is_not_match(self):
Expand All @@ -599,6 +601,14 @@ def test_if_is_not_variable_missing(self):
@setup({'template': '{% if foo is not bar %}yes{% else %}no{% endif %}'})
def test_if_is_not_both_variables_missing(self):
output = self.engine.render_to_string('template', {})
self.assertEqual(output, 'yes')

@setup({'if-tag-invalid-member': '{% if x.non_existent == None %}yes{% else %}no{% endif %}'})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

member -> attribute

def test_if_tag_invalid_member(self):
"""
Invalid attributes should not be regarded as equal to None
"""
output = self.engine.render_to_string('if-tag-invalid-member', {'x': TestObj()})
self.assertEqual(output, 'no')


Expand Down
5 changes: 1 addition & 4 deletions tests/template_tests/syntax_tests/test_invalid_string.py
Expand Up @@ -9,10 +9,7 @@ class InvalidStringTests(SimpleTestCase):
@setup({'invalidstr01': '{{ var|default:"Foo" }}'})
def test_invalidstr01(self):
output = self.engine.render_to_string('invalidstr01')
if self.engine.string_if_invalid:
self.assertEqual(output, 'INVALID')
else:
self.assertEqual(output, 'Foo')
self.assertEqual(output, 'Foo')

@setup({'invalidstr02': '{{ var|default_if_none:"Foo" }}'})
def test_invalidstr02(self):
Expand Down
3 changes: 2 additions & 1 deletion tests/template_tests/syntax_tests/test_url.py
Expand Up @@ -166,7 +166,8 @@ def test_url_fail09(self):

@setup({'url-fail11': '{% url named_url %}'})
def test_url_fail11(self):
with self.assertRaises(NoReverseMatch):
with self.assertRaisesRegex(TemplateSyntaxError,
"Variable named_url is not defined"):
self.engine.render_to_string('url-fail11')

@setup({'url-fail12': '{% url named_url %}'})
Expand Down
4 changes: 2 additions & 2 deletions tests/template_tests/tests.py
Expand Up @@ -22,7 +22,7 @@ def test_url_reverse_no_settings_module(self):
#9005 -- url tag shouldn't require settings.SETTINGS_MODULE to
be set.
"""
t = Engine(debug=True).from_string('{% url will_not_match %}')
t = Engine(debug=True).from_string('{% url "will_not_match" %}')
c = Context()
with self.assertRaises(NoReverseMatch):
t.render(c)
Expand All @@ -33,7 +33,7 @@ def test_url_reverse_view_name(self):
exception.
"""
t = Engine().from_string('{% url will_not_match %}')
c = Context()
c = Context({"will_not_match": "not_reversible"})
try:
t.render(c)
except NoReverseMatch:
Expand Down