#30197 closed New feature (wontfix)
Template variable resolution with class objects
Reported by: | Alex Epshteyn | Owned by: | nobody |
---|---|---|---|
Component: | Template system | Version: | dev |
Severity: | Normal | Keywords: | template, variable, resolve |
Cc: | Triage Stage: | Unreviewed | |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
The behavior of if callable(current):
... current = current()
in 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:
@register.filter() def type_name(someClass): return someClass.__name__
Using this filter in a template like:
<div>The class name is {{ 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
*:
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:
if callable(current) and not inspect.isclass(current): if getattr(current, 'do_not_call_in_templates', False): pass # etc...
or
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.
Change History (8)
follow-up: 3 comment:1 by , 6 years ago
Easy pickings: | unset |
---|
follow-up: 5 comment:2 by , 6 years ago
Resolution: | → duplicate |
---|---|
Status: | new → closed |
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.
comment:3 by , 6 years ago
Replying to 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 Google App Engine and foo
is an instance of 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.
comment:4 by , 6 years ago
Description: | modified (diff) |
---|
follow-up: 6 comment:5 by , 6 years ago
Resolution: | duplicate |
---|---|
Status: | closed → new |
Replying to 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 my reply to Tim Graham's comment).
Furthermore, the existing the do_not_call_in_templates
workaround requires modifying objects in the application layer, which breaks the fundamental Django design philosophies of Loose coupling (by moving some template-specific concerns from the template layer to the application layer), Less code, and 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" (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.
{% call foo %}
instead of
{{ 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 autoescape, that would allow turning off invoking callable variables in a particular template block. For example:
{% 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:
- It's not a duplicate of #15791
- I've presented some valid arguments that shouldn't be summarily dismissed without getting feedback from the whole community:
- that it does not make sense to implicitly invoke a callable during variable resolution unless the previous
bit
inself.lookups
was an instance and thecurrent
bit is a method that should be invoked on that instance. - we should encourage adherence to the "Explicit is better than implicit" principle.
- that it does not make sense to implicitly invoke a callable during variable resolution unless the previous
comment:6 by , 6 years ago
follow-up: 8 comment:7 by , 6 years ago
Resolution: | → wontfix |
---|---|
Status: | new → closed |
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.
comment:8 by , 6 years ago
Replying to 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.
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.