[Django] #30197: Template variable resolution with class objects
Django
-------------------------------------+-------------------------------------
Reporter: aepshteyn | Owner: nobody
Type: New | Status: new
feature |
Component: Template | Version: master
system | Keywords: template, variable,
Severity: Normal | resolve
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 1
UI/UX: 0 |
-------------------------------------+-------------------------------------
The behavior of `if callable(current):` ... `current = current()` in
[https://github.com/django/django/blob/21ff23bfeb4014bceaa3df27677fb68409c0634d/django/template/base.py#L851
django.template.base.Variable._resolve_lookup] prevents using a function
(or a class!) as a **value** in a template.
This behavior only makes sense when the previous `bit` in `self.lookups`
was an //instance// and the `current` bit is a //method// that should be
invoked on that instance. I would argue that it //never// makes sense in
any other case (for example,
[https://stackoverflow.com/q/6861601/1965404]).
== Example
Suppose you want to write a filter that returns the name of a class:
{{{#!python
@register.filter()
def type_name(someClass):
return someClass.__name__
}}}
Using this filter in a template like:
{{{#!django
<div>Foo.Bar is an instance of {{ foo.bar|type_name }}</div>
}}}
might seem valid, but it turns out that this expression will //never//
work as intended, due to the implementation of `Variable._resolve_lookup`
— the filter we defined in this example will pretty much always return an
empty string (or whatever value you configured for your app's
`string_if_invalid` setting).
For example, if `foo.bar` is `<type 'basestring'>`, invoking it as a
callable will raise **`TypeError: The basestring type cannot be
instantiated`**, and if `foo.bar` is some other type that is actually
instantiable (e.g. `<type 'str'>`, or a user-defined class), our filter
will receive an //instance// of that class (rather than the class itself),
which will almost-certainly lead to an exception like **`AttributeError:
'str' object has no attribute '__name__'`**
== Approaches tried in the past
The workaround introduced by #15791 allows setting an attribute named
`do_not_call_in_templates` on a callable object to be able to use the
//object itself// as the value (rather than the //value returned from
invoking it//).
However, this seems like a dirty hack, and furthermore, in many cases you
may not want to (or even be able to) set an attribute on that object (e.g.
if this object is a class that comes from a library and you have no
control of it).
=== Current implementation of `Variable._resolve_lookup`^*^:
{{{#!python
def _resolve_lookup(self, context):
"""
Perform resolution of a real variable (i.e. not a literal) against the
given context.
"""
current = context
try: # catch-all for silent variable failures
for bit in self.lookups:
try: # dictionary lookup
current = current[bit]
# (followed by nested try/except blocks for attribute lookup
and list-index lookup)
# ...
# *** and at last ***:
if callable(current):
if getattr(current, 'do_not_call_in_templates', False):
pass
elif getattr(current, 'alters_data', False):
current = context.template.engine.string_if_invalid
else:
try: # method call (assuming no args required)
current = current()
# etc...
# etc...
return current
}}}
^*^ //abbreviated for clarity//
== Proposed solution
At the very least, I think it would make sense to add a check for
`inspect.isclass` to the `if callable(current):` ... `current = current()`
block of this method.
For example:
{{{#!python
if callable(current) and not inspect.isclass(current):
if getattr(current, 'do_not_call_in_templates', False):
pass
# etc...
}}}
or
{{{#!python
if callable(current):
if getattr(current, 'do_not_call_in_templates', False) or
inspect.isclass(current):
pass
# etc...
}}}
However, ideally, it would be even better to check whether the previous
`bit` in `self.lookups` was an //instance// and the `current` bit is a
//method// supported by the class of that instance before trying to invoke
`current` as a callable.
--
Ticket URL: <https://code.djangoproject.com/ticket/30197>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: aepshteyn | Owner: nobody
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: template, variable, | Triage Stage:
resolve | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Changes (by Tim Graham):
* easy: 1 => 0
Comment:
Could you detail your use case a little more? What's `Foo.Bar` some nested
class? Also, the discussion of `<type 'basestring'>` is outdated since
basestring is removed in Python 3. If you can present a patch with a test,
we can take a look, although I'm not sure it'll be backwards compatible. I
guess a documentation change may be needed to describe the new behavior.
--
Ticket URL: <https://code.djangoproject.com/ticket/30197#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: aepshteyn | Owner: nobody
Component: Template system | Version: master
Keywords: template, variable, | Triage Stage:
resolve | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => duplicate
Comment:
I think we have to close this as a duplicate of #15791, which introduced
`do_not_call_in_templates`, and just say ''"Use that"''.
(See also #29382, #29306, ...)
In particular, people **will be** passing classes to templates and relying
on their being ''called'' such that `...or inspect.isclass(current): pass`
would be a breaking change.
The alternative natural approach to setting `do_not_call_in_templates` is
to move the logic resolving the class name up into the scope where you
prepare the context data.
--
Ticket URL: <https://code.djangoproject.com/ticket/30197#comment:2>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: closed
Component: Template system | Version: master
Severity: Normal | Resolution: duplicate
Keywords: template, variable, | Triage Stage:
resolve | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Comment (by Alex Epshteyn):
Replying to [comment:1 Tim Graham]:
> Could you detail your use case a little more? What's `Foo.Bar` some
nested class? Also, the discussion of `<type 'basestring'>` is outdated
since basestring is removed in Python 3. If you can present a patch with a
test, we can take a look, although I'm not sure it'll be backwards
compatible. I guess a documentation change may be needed to describe the
new behavior.
"Foo.Bar" is just a label used in the example template -- it could be
anything, really (I'll update the example to avoid this confusion). The
actual class is not known until run-time (otherwise we wouldn't need to
use our `type_name` filter in the first place :)). Let's just say that
`foo` is some arbitrary object that has an attribute named "bar", whose
value is a class.
In my particular use case, the code is running on the Python 2.7 runtime
of [https://cloud.google.com/appengine/docs/python/ Google App Engine] and
`foo` is an instance of
[https://cloud.google.com/appengine/docs/standard/python/refdocs/google.appengine.ext.db#google.appengine.ext.db.Property
google.appengine.ext.db.Property], and `bar` is its "data_type" attribute
(whose value is the Python class representing this datastore property's
storage type). My template is listing all the properties defined for a
particular datastore model class, including their storage type and other
metadata. For many subclasses of `db.Property`, the `data_type` is
`basestring`, but that's not really the point -- this template wouldn't
work regardless of the data type and regardless of whether it's running on
Python 2 or 3, because de-referencing this variable in a template returns
a new instance of the data type rather than the data type itself.
--
Ticket URL: <https://code.djangoproject.com/ticket/30197#comment:3>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: closed
Component: Template system | Version: master
Severity: Normal | Resolution: duplicate
Keywords: template, variable, | Triage Stage:
resolve | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Description changed by Alex Epshteyn:
Old description:
New description:
The behavior of `if callable(current):` ... `current = current()` in
[https://github.com/django/django/blob/21ff23bfeb4014bceaa3df27677fb68409c0634d/django/template/base.py#L851
django.template.base.Variable._resolve_lookup] prevents using a function
(or a class!) as a **value** in a template.
This behavior only makes sense when the previous `bit` in `self.lookups`
was an //instance// and the `current` bit is a //method// that should be
invoked on that instance. I would argue that it //never// makes sense in
any other case (for example,
[https://stackoverflow.com/q/6861601/1965404]).
== Example
Suppose you want to write a filter that returns the name of a class:
{{{#!python
@register.filter()
def type_name(someClass):
return someClass.__name__
}}}
Using this filter in a template like:
{{{#!django
<div>The class name is {{ foo.bar|type_name }}</div>
== Proposed solution
--
--
Ticket URL: <https://code.djangoproject.com/ticket/30197#comment:4>
Django
-------------------------------------+-------------------------------------
Component: Template system | Version: master
Severity: Normal | Resolution:
resolve | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* status: closed => new
* resolution: duplicate =>
Comment:
Replying to [comment:2 Carlton Gibson]:
> I think we have to close this as a duplicate of #15791, which introduced
`do_not_call_in_templates`, and just say ''"Use that"''.
> (See also #29382, #29306, ...)
>
> In particular, people **will be** passing classes to templates and
relying on their being ''called'' such that `...or
inspect.isclass(current): pass` would be a breaking change.
>
> The alternative natural approach to setting `do_not_call_in_templates`
is to move the logic resolving the class name up into the scope where you
prepare the context data.
>
I agree that this would be a breaking change, but **I don't agree that
this is a duplicate of #15791**, because in some cases it may not be
possible to set `do_not_call_in_templates` on the callable object (see
[https://code.djangoproject.com/ticket/30197#comment:3 my reply to Tim
Graham's comment]).
Furthermore, the existing the `do_not_call_in_templates` workaround
requires making changes to objects in the application layer, which
**breaks the fundamental [https://django.readthedocs.io/en/stable/misc
/design-philosophies.html Django design philosophies]** of
//[https://django.readthedocs.io/en/stable/misc/design-philosophies.html
#loose-coupling "Loose coupling"]// (by moving some template-specific
concerns from the template layer to the application layer),
//[https://django.readthedocs.io/en/stable/misc/design-philosophies.html
#less-code "Less code"]//, and
//[https://django.readthedocs.io/en/stable/misc/design-philosophies.html
#explicit-is-better-than-implicit "Explicit is better than implicit"].
Therefore, I would argue that **a breaking change like this is
justified**, because it would fix a serious design flaw of the template
variable resolution algorithm. I've been bit by this problem several
times, which has caused much frustration and hours wasted on debugging.
And I'm not the only one (as evidenced by the prevalence of tickets opened
for this issue).
I understand that since this behavior has been there for many years, there
must be some existing templates that rely on code like `{{ foo }}` to
display a value returned by the function `foo`, but this is just poor
design, which goes against the core Python and Django tenet of //"Explicit
is better than implicit"// ([https://www.python.org/dev/peps/pep-0020/
"PEP 20"]).
One idea would be to add a new built-in tag for this use-case, which would
**make it explicit that the author wants to display the result of a
function**, e.g.
{{{#!django
{% call foo %}
}}}
instead of
{{{#!django
{{ foo }}
}}}
However, I also understand not wanting to make a breaking change, so here
is my new proposal:
== Proposed solution (non-breaking change!)
Introduce a new built-in tag similar to
[https://django.readthedocs.io/en/stable/ref/templates/builtins.html#autoescape
autoescape], that would allow turning off invoking callable variables in a
particular template block. For example:
{{{#!django
{% callables off %}
<div>The class name is {{ foo.bar|type_name }}</div>
{% endcallables %}
}}}
The introduction of this new `callables [on|off]` tag would be a **non-
breaking** way to implement this feature. The only problem with it is
that it doesn't exactly meet the //"Less code"// criterion :)
----
I'll create a new feature request ticket for my new idea, but I'm also
reopening the current ticket because I believe that:
1. It's **not a duplicate** of #15791
2. I've presented some valid arguments that shouldn't be summarily
dismissed without getting feedback from the whole community:
a. that it does not make sense to implicitly invoke a callable during
variable resolution unless the previous `bit` in `self.lookups` was an
//instance// and the `current` bit is a //method// that should be invoked
on that instance.
b. we should encourage adherence to the //"Explicit is better than
implicit"// principle.
--
Ticket URL: <https://code.djangoproject.com/ticket/30197#comment:5>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: new
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: template, variable, | Triage Stage:
resolve | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Comment (by Alex Epshteyn):
Replying to [comment:5 Alex Epshteyn]:
> I'll create a new feature request ticket for my new idea
See #30205
--
Ticket URL: <https://code.djangoproject.com/ticket/30197#comment:6>
Django
-------------------------------------+-------------------------------------
Component: Template system | Version: master
Keywords: template, variable, | Triage Stage:
resolve | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => wontfix
Comment:
Please use the DevelopersMailingList to get feedback from the whole
community. That's where design decisions are made and consensus to reopen
tickets is gathered.
--
Ticket URL: <https://code.djangoproject.com/ticket/30197#comment:7>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: closed
Component: Template system | Version: master
Severity: Normal | Resolution: wontfix
Keywords: template, variable, | Triage Stage:
resolve | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Comment (by Alex Epshteyn):
Replying to [comment:7 Tim Graham]:
> Please use the DevelopersMailingList to get feedback from the whole
community. That's where design decisions are made and consensus to reopen
tickets is gathered.
Got it, thanks. I just posted a new topic to the mailing list.
--
Ticket URL: <https://code.djangoproject.com/ticket/30197#comment:8>