[Django] #28782: Template variable resolution on objects that are no mappings but implement __getitem__
Django
__getitem__
-------------------------------------+-------------------------------------
Reporter: Frank | Owner: Frank Sachsenheim
Sachsenheim |
Type: Bug | Status: assigned
Component: Template | Version: master
system |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
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
[https://docs.python.org/3/glossary.html#term-mapping 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
[https://www.python.org/dev/peps/pep-3119/#rationale 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?
--
Ticket URL: <https://code.djangoproject.com/ticket/28782>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
__getitem__
-------------------------------------+-------------------------------------
| Sachsenheim
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/28782#comment:1>
Django
__getitem__
-------------------------------------+-------------------------------------
| Sachsenheim
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Frank Sachsenheim):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/28782#comment:2>
Django
__getitem__
-------------------------------------+-------------------------------------
| Sachsenheim
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Frank Sachsenheim):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/28782#comment:3>
Django
__getitem__
-------------------------------------+-------------------------------------
| Sachsenheim
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Unreviewed => Someday/Maybe
Comment:
[https://groups.google.com/d/topic/django-developers/zFh-MF-
j1lc/discussion django-developers discussion] and
[https://github.com/django/django/pull/9341 PR] (not passing tests). It
looks like it's still unclear if this is feasible and a good idea.
--
Ticket URL: <https://code.djangoproject.com/ticket/28782#comment:4>