Context.update doesn't do what it says it does

[Chris Beaven]

Context.update() currently looks like:

    def update(self, other_dict):
        "Like dict.update(). Pushes an entire dictionary's keys and values onto the context."
        self.dicts = [other_dict] + self.dicts

but really to act like dict.update() it should it look like:

        self.dicts[0].update(other_dict)

[Chris Beaven]

This is a change in functionality, so I'll run it passed someone else first.

This bug was discovered as part of discussion in #3523

[arnodel@…]

I made this exact change to the Context patch when I wrote my own patch to the {% for ... in ... %} tag. I checked then for occurences in Context.update in the django source and the only one I found was in django.template.loader, function render_to_string(), line 103. With this patch this should be adapted slightly so that the context_instance parameter of the function doesn't get polluted with the context of dictionary. In fact as it is this function seems buggy as it changes the value of the parameter context_instance if it is not None.

I didn't find any other reference to Context.update().

Another point: I think that the list Context.dicts is the wrong way round. Lists in python are designed so that it is easy to add or remove elements at the end, not at the front. So the current context should be Context.dict[-1], popping a context then should be Context.dict.pop(), and pushing a context should be Context.dict.append(new_dict). As it is every time a context is pushed or popped, the whole context list (Context.dicts) has to be rebuilt.

Sorry if this is not the right place to make theses comments.
--
Arnaud

[Chris Beaven]

Regarding reversing the list in Context.dicts, although it would make popping and pushing slightly faster, it would slow down getting a variable's value a lot (based on some timeit tests that I just did). It has probably been done the way it is on purpose because of this.

And on to the main issue, reading the full docstring, Context.update probably does do what is says it does, but I'm still not convinced it is doing what it should. As a compromise, I wrote a patch to keep the current behaviour default and introduce a new argument for update so that you can turn off the pushing behaviour and just update the most recent context dictionary instead.

[Chris Beaven]

(This time it's fully backwards compatible)

[arnodel@…]

I believe the reason why you see slowdown in variable lookup when implementing my suggestion is because Context.__getitem__ needs to be changed when the dicts list order is reversed.

If you reverse the list, you also need to reverse variable lookup. ATM we have:

    def __getitem__(self, key):
        "Get a variable's value, starting at the current context and going upward"
        for d in self.dicts:
            if key in d:
                return d[key]
        raise KeyError(key)

for d in self.dicts should be changed to for d in reversed(self.dicts) (or some python 2.3 compatible construct). Then there shouldn't be any noticeable difference in variable lookup with the other version (as lists elements are accessed in constant time in Python). Unfortunately I am not in a position to test this right now.

--
Arnaud

[Chris Beaven]

In discussing #3523, Brian said

Technically the change in 3529 should support dict.update's "I take
either a sequence of len 2 tuples, or a dict" also- meaning if update
is going to be made closer to dict behavior, the intermediate dict
isn't needed (straight izip in that case).

[Jacob]

Reversing the list is the wrong thing to do -- it penalizes lookups (especially on 2.3).

However, I think I'm leaning towards indeed making update() directly update the top-level dict. That *might* lead to a few popped-too-many-times exceptions in user code, so we'll need a note on BackwardsIncompatibleChanges.

[David Danier <goliath.mailinglist@…>]

I don't think adding the second parameter ("push") to get the current behavior is really necessary. If someone needs this behavior he could simply use:

context.push() 
context.update(d)

...which would be much easier to read anyway.

About the usage of Context.update() in django.template.loader.render_to_string() and django.template.context.RequestContext:
I'm not sure if pushing the context here is the wrong thing anyway. For example: Currently if you have some context_processors, all the dictionaries they provide are "copied" into the Context using Context.update(). As the implementation does a push every context_processor gets its own item in Context.dicts. This may make it more difficult to find bugs that would raise ContextPopException() when using Context.pop(). Additionally Context.getitem() has to iterate over all items in Context.dicts to find the right value. As some people have many context_processors installed this adds many entries in Context.dicts, which might be a lot more than you would normally have when using Context.push() only in template-tags (e.g. {% for ... %}). So this might even be positive when thinking about performance (depending on how ofter the context gets updates and getitem is called).

So I would apply the first patch (​http://code.djangoproject.com/attachment/ticket/3529/context_update.patch) and only update code using Context.update() when really necessary, which IMHO does not include:

• django.template.loader.render_to_string()
• django.template.context.RequestContext

Both would work without push() and users could benefit from getting ContextPopException on the right spot (not only when using the self-written tag for more than len(settings.TEMPLATE_CONTEXT_PROCESSORS) [ + 1 ] times).

[maxp]

I just spent a lot of time trying to figure out why my context was clobbered and eventually tracked it down to one of my tags doing the following:

context.push() 
context.update(d)
 ...
context.pop()

I think this is the obvious behaviour (e.g I do not think that update implies push).

If it is too late for this to change it should at least be properly documented.

[Chris Beaven]

Yeah, there's no way this is going to change now. But I agree, the current behaviour needs documenting [at least in the docstring].

[Chris Beaven]

I'd like to see the documentation have a separate block which explains the Context.update behaviour -- it's likely to get overlooked otherwise.

[ggetzie]

Moved the documentation for Context.update to its own block

[Chris Beaven]

(In [14689]) Fixes #3529 -- more explicit documentation about Context.update. Thanks for the patch, ggetzie.