Opened 9 years ago

Closed 7 years ago

Last modified 7 years ago

#2327 closed defect (wontfix)

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

Reported by: mir@… Owned by: mir
Component: Documentation Version: master
Severity: normal Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: yes
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

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.

Change History (8)

comment:1 Changed 8 years ago by jeroen@…

  • Triage Stage changed from Unreviewed to Accepted

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

comment:2 Changed 8 years ago by Rob Hunter <robertjhunter@…>

  • Triage Stage changed from Accepted to Design decision needed

comment:3 Changed 8 years ago by SmileyChris

  • Component changed from Generic views to Documentation
  • Needs documentation set
  • Summary changed from views.generic.create_update.update_object etc. overwrite dict provided by last context processor to Need a warning that dictionaries passed from a custom context processor may be modified
  • Triage Stage changed from Design decision needed to Accepted

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

comment:4 follow-up: Changed 8 years ago by mtredinnick

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.

comment:5 in reply to: ↑ 4 Changed 8 years ago by mir

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

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

comment:6 Changed 7 years ago by ubernostrum

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

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.

comment:7 Changed 7 years ago by mir

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.

comment:8 Changed 7 years ago by ubernostrum

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.

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