Opened 9 years ago

Closed 5 years ago

#3529 closed (fixed)

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

Reported by: SmileyChris Owned by: ggetzie
Component: Documentation Version: master
Severity: Keywords:
Cc: goliath.mailinglist@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:


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:


Attachments (3)

context_update.patch (576 bytes) - added by SmileyChris 9 years ago.
context_update.2.patch (1.0 KB) - added by SmileyChris 8 years ago.
context-doc.diff (1.4 KB) - added by ggetzie 5 years ago.
Moved the documentation for Context.update to its own block

Download all attachments as: .zip

Change History (22)

Changed 9 years ago by SmileyChris

comment:1 Changed 9 years ago by SmileyChris

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Design decision needed

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

comment:2 Changed 9 years ago by 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.

comment:3 Changed 8 years ago by SmileyChris

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.

Changed 8 years ago by SmileyChris

comment:4 Changed 8 years ago by SmileyChris

(This time it's fully backwards compatible)

comment:5 Changed 8 years ago by (removed)

  • Cc ferringb@… added

comment:6 Changed 8 years ago by 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.


comment:7 Changed 8 years ago by SmileyChris

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

comment:8 Changed 8 years ago by jacob

  • Triage Stage changed from Design decision needed to Accepted

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.

comment:9 Changed 8 years ago by David Danier <goliath.mailinglist@…>

  • Cc goliath.mailinglist@… added

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:


...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 ( 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).

comment:10 Changed 7 years ago by 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:


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.

comment:11 Changed 6 years ago by SmileyChris

  • Has patch unset
  • Needs documentation set

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

comment:12 Changed 6 years ago by (removed)

  • Cc ferringb@… removed

comment:13 Changed 5 years ago by ggetzie

  • Owner changed from nobody to ggetzie
  • Status changed from new to assigned

comment:14 Changed 5 years ago by SmileyChris

  • Component changed from Template system to Documentation

comment:15 Changed 5 years ago by ggetzie

  • Has patch set

comment:16 Changed 5 years ago by SmileyChris

  • Patch needs improvement set

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

Changed 5 years ago by ggetzie

Moved the documentation for Context.update to its own block

comment:17 Changed 5 years ago by ggetzie

  • Patch needs improvement unset

comment:18 Changed 5 years ago by SmileyChris

  • Needs documentation unset
  • Triage Stage changed from Accepted to Ready for checkin

comment:19 Changed 5 years ago by SmileyChris

  • Resolution set to fixed
  • Status changed from assigned to closed

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

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