Need a warning that dictionaries passed from a custom context processor may be modified

[mir@…]

An unexpected interaction between RequestContext and update_view modifies the dict the last ContextProcessor provides when you have extra_context.

update_object contains this code to build the context for the template (lines 140ff)

    c = RequestContext(request, {
        'form': form,
        template_object_name: object,
    }, context_processors)
    for key, value in extra_context.items():
        if callable(value):
            c[key] = value()
        else:
            c[key] = value

The first line creates the request context. RequestContext evaluates the context processors and puts the content of the last context processor on top of its stack, since Context.update does

        self.dicts = [other_dict] + self.dicts

Back in update_object, the last lines directly assign to this dict.

My context processor happend to do something like this:

NAVIGATION_CONTEXT = {...}

...

def navigation_context_processor(request):
    return NAVIGATION_CONTEXT

Since update_object now modifies NAVIGATION_CONTEXT, this led to strange effects.

In my opinion, this a bug of update_object, but you may consider it only a documentation issue. But please let's at least document it.

The other generic views have the same problem.

[jeroen@…]

Confirmed this issue - could bite you also when you decide to add a key named 'form' or 'template_object_name' to extra_dict (why you'd want to do that is beyond me, but well ...). Perhaps it would be better to modify e.g. update_object to read:

data = {}
for key, value in extra_context.items():
    if key in ['form', 'template_object_name']:
        raise KeyError, "invalid key for extra_context"
    if callable(value):
        data[key] = value()
    else:
        data[key] = value
data['form'] = form
data['template_object_name'] = object
c = RequestContext(request, data, context_processors)

Still, this would require documenting that extra_context should not contain certain keys, like 'form' or 'template_object_name'.

[Chris Beaven]

jeroen's issue is actually a different one. I'll ignore that and focus on the original ticket.

The issue here isn't actually anything to do with generic views -- it's the fact that mir was passing a mutable object in the context processor. I think the only thing that needs to change here is documentation. A short paragraph in the templates_python.txt doc under the Writing your own context processors section would be good. It just needs to warn users about the danger of passing a mutable object like a dict without copying (but better explained that I can :).

[Malcolm Tredinnick]

So you want a warning to say that normal Python behaviour will apply here? The problem with that is where do you stop. We tend to highlight wherever normal Python behaviour does not apply (for example, that the queryset parameter to generic views is evaluated afresh each time).

Yes, this (passing mutable arguments) is something that bites everybody when they start trying to be tricky, but it's a standard Python thing. And the problem is that a caution, by definition, has to be something you notice, so it's kind of contradictory to ask for a non-obtrusive patch here.

Right now, I'm between -1 and -0 on this, but if you want to try out a patch that doesn't look really annoying and doesn't say the equivalent of "gravity may apply on this planet", give it a shot and I'm willing to change my mind.

[Michael Radziej]

Replying to mtredinnick:

It's a bit different here. I don't call update_object with a mutable argument and wonder how it got modified. I return a mutable object in my context processor and update_object chooses to modify it without any particular reason, just as a side effect.

I think create_update should simply push extra_context on top of the context stack. I'll prepare a patch, minding gravity as much as possible.

Replying to jeroen:
I consider this a different problem. And, if you put 'form' into extra_context, you'll probably know what you do (and refrain).

[James Bennett]

Following up on what Malcolm said: the stack-based implementation of Context isn't meant to imply that it's safe to sling something into a Context and expect it never to be modified. And since Context goes to some lengths to otherwise behave like a mapping type, it should be expected that it's mutable and that code further down the line may change some values. Given that, I'm marking this wontfix.

[Michael Radziej]

Well, "just for the records", I still consider it hostile to users.

Of course it's possible to modify a dict in a called function, but I consider it a bad practice to do this as an unexpected side effect.

[James Bennett]

I guess I'm confused here. You have a dictionary which came out of the context processor, and another dictionary that you're explicitly passing in extra_context for the express purpose of updating values in the first dictionary. And you're expecting it won't do that? AFAICT, trying to magically prevent the dict from changing in this case would be problematic from both an intuitive and a development standpoint.