Opened 17 years ago

Closed 13 years ago

#3529 closed (fixed)

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

Reported by: Chris Beaven Owned by: ggetzie
Component: Documentation Version: dev
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: no UI/UX: no

Description

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)

Attachments (3)

context_update.patch (576 bytes ) - added by Chris Beaven 17 years ago.
context_update.2.patch (1.0 KB ) - added by Chris Beaven 17 years ago.
context-doc.diff (1.4 KB ) - added by ggetzie 13 years ago.
Moved the documentation for Context.update to its own block

Download all attachments as: .zip

Change History (22)

by Chris Beaven, 17 years ago

Attachment: context_update.patch added

comment:1 by Chris Beaven, 17 years ago

Triage Stage: UnreviewedDesign 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 by arnodel@…, 17 years ago

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

comment:3 by Chris Beaven, 17 years ago

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.

by Chris Beaven, 17 years ago

Attachment: context_update.2.patch added

comment:4 by Chris Beaven, 17 years ago

(This time it's fully backwards compatible)

comment:5 by (removed), 17 years ago

Cc: ferringb@… added

comment:6 by arnodel@…, 17 years ago

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

comment:7 by Chris Beaven, 17 years ago

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 by Jacob, 16 years ago

Triage Stage: Design decision neededAccepted

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 by David Danier <goliath.mailinglist@…>, 16 years ago

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:

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

comment:10 by maxp, 15 years ago

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.

comment:11 by Chris Beaven, 15 years ago

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 by (removed), 14 years ago

Cc: ferringb@… removed

comment:13 by ggetzie, 13 years ago

Owner: changed from nobody to ggetzie
Status: newassigned

comment:14 by Chris Beaven, 13 years ago

Component: Template systemDocumentation

comment:15 by ggetzie, 13 years ago

Has patch: set

comment:16 by Chris Beaven, 13 years ago

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.

by ggetzie, 13 years ago

Attachment: context-doc.diff added

Moved the documentation for Context.update to its own block

comment:17 by ggetzie, 13 years ago

Patch needs improvement: unset

comment:18 by Chris Beaven, 13 years ago

Needs documentation: unset
Triage Stage: AcceptedReady for checkin

comment:19 by Chris Beaven, 13 years ago

Resolution: fixed
Status: assignedclosed

(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