Opened 6 years ago

Last modified 6 years ago

#28782 assigned Bug

Template variable resolution on objects that are no mappings but implement __getitem__

Reported by: Frank Sachsenheim Owned by: Frank Sachsenheim
Component: Template system Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Someday/Maybe
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

there are classes in the Pythoniverse that implement a __getitem__ method for the sake of syntactical sweetness, but they are no mappings. (in my case it's a wrapper around an xml document where __getitem__ returns the result of xpath evaluations. there are other use-cases around, but i don't remember any atm.).
this causes a lookup in a template rendering context on an attribute of such instances to return an unexpected value, because foo[bar] doesn't necessarily raise an exception and returns some value, while foo.bar was intended. this is caused by the very implicit implementation of django.template.base.Variable._resolve_lookup.

my approach to solve this issue is to refactor that method to an explicit behaviour mainly based on type checks, so a member lookup is only performed on instances that base on collections.abc.Mapping which any implementation of a mapping should. my rationale is that the documentations mentions a dictionary (though mapping would be more precise term) lookup, not a lookup via __getitem__, and dictionary/mapping objects should base on the mentioned abstract class (the why is elaborated in PEP 3119).

beside solving my problem, i would argue that in doubt "explicit is better than implicit" excels "it's better to ask forgiveness than to ask permission" and the explicit approach is more comprehensible.

atm i got this working for all tests in template_tests, but still need to figure out why some more complex tests fail.

so, i could need some feedback at this point before i continue. one question is whether access to non-mappings via __getitem__ should be supported as last resort with a deprecation warning.

two related issues came up while working on it:

1) i didn't research why this was implemented, but the possiblity to lookup attributes on context objects is rather ambigious imo. could this design decision be reverted? (sidenote: using SimpleNamespace objects as context seems reasonable though, and they can easily be converted to a mapping with vars.)
2) it seems to me that django.template.context.Basecontext could be based on collections.ChainMap and it may then even allow to simplify the …._resolve_lookup implementation. shall i give it a shot while i'm at it?

Change History (4)

comment:1 by Tim Graham, 6 years ago

You're unlikely get a helpful response in this ticket tracker. You could try a post to the DevelopersMailingList but I'm not sure if anyone who worked on the template system in the early days of Django is still active. I might be able to answer some of your questions by researching history with git blame but I think you could do that just as well. It might be helpful to see your work-in-progress code, also.

Given that the template system is over 10 years old, my gut says the risk of regressions for this change is high and you might be better off adapting your code to the current behavior. For example, maybe you could wrap the objects in question in a class that will do the lookup that you want.

comment:2 by Frank Sachsenheim, 6 years ago

thanks for your response, Tim.

You could try a post to the DevelopersMailingList

i'll do that. though i need to create another account for that. :-/

but I think you could do that just as well.

yep.

It might be helpful to see your work-in-progress code, also.

as a pull request on github or otherwise?

you might be better off adapting your code to the current behavior. For example, maybe you could wrap the objects in question in a class that will do the lookup that you want.

yes, i thought of this. but from a rather dogmatic, pythonic view the code as it is is 'wrong'. Django has maintained a reasonable release policy over the years that allows deprecations. again, from a documentation's point of view parts of the behaviour couldn't be taken as granted. Python has evolved, and with the support of only the recent versions, there's an opportunity to adapt to its enhancements. after all, Django has an emphasis on clean code and this is a constraint for the longevity of the project.

comment:3 by Frank Sachsenheim, 6 years ago

a thought i just had: shouldn't it be the context object's responsibilty to resolve a value from a variable? while the latter is just a descriptor parsed from the template. this would allow easy customization.

comment:4 by Tim Graham, 6 years ago

Triage Stage: UnreviewedSomeday/Maybe

django-developers discussion and PR (not passing tests). It looks like it's still unclear if this is feasible and a good idea.

Note: See TracTickets for help on using tickets.
Back to Top