Template variable resolution with class objects

[Alex Epshteyn]

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.

[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.

[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.

[Alex Epshteyn]

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.

[Alex Epshteyn]

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 making changes to 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:

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:
   1. 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.
   2. we should encourage adherence to the "Explicit is better than implicit" principle.

[Alex Epshteyn]

Replying to Alex Epshteyn:

I'll create a new feature request ticket for my new idea

See #30205

[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.

[Alex Epshteyn]

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.