Opened 12 years ago

Closed 12 years ago

#18272 closed Bug (wontfix)

ModelChoiceField's queryset attribute persists between two form instances

Reported by: Baptiste Mispelon Owned by: Spark23
Component: Forms Version: dev
Severity: Normal Keywords: modelchoicefield deepcopy
Cc: Triage Stage: Design decision needed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Setup

Using the models from the tutorial:

from django.db import models

class Poll(models.Model):
    question = models.CharField(max_length=200)
    pub_date = models.DateTimeField('date published')

class Choice(models.Model):
    poll = models.ForeignKey(Poll)
    choice = models.CharField(max_length=200)
    votes = models.IntegerField()

And the following form:

from django import forms
from polls.models import Poll

class ChoiceForm(forms.Form):
    poll = forms.ModelChoiceField(queryset=Poll.objects.all())
    
    def __init__(self, *args, **kwargs):
        """Change the choices attribute of the field `poll`."""
        super(ChoiceForm, self).__init__(*args, **kwargs)
        queryset = self.fields['poll'].queryset
        choices = [(poll.pk, poll.pk) for poll in queryset]
        self.fields['poll'].choices = choices

Bug

When doing so, the queryset gets "stuck" on what it was the first time the form was instanciated: added Poll objects don't show up in it, and deleted polls are still present.

from datetime import datetime
from polls.forms import ChoiceForm
from polls.models import Poll

Poll.objects.all().delete() # Just making sure.

f = ChoiceForm()
print f['poll'].as_widget()
# '<select name="poll" id="id_poll">\n</select>'

Poll.objects.create(question="What is your favourite colour?", pub_date=datetime.now())

f = ChoiceForm()
print f['poll'].as_widget()
# Expected: '<select name="poll" id="id_poll">\n<option value="1">1</option>\n</select>'
# Result: '<select name="poll" id="id_poll">\n</select>'

Workaround

One workaround is to copy the queryset before iterating over it:

queryset = self.fields['poll'].queryset.all()

Analysis

I believe the issue lies in ModelChoiceField.__deepcopy__(), in particular the line:

result.queryset = result.queryset

queryset being a property, this ends up doing (among other things which I think aren't relevant here):

result._queryset = result._queryset

And since _queryset has been iterated over, its cache is filled and it gets attached onto the deepcopied instance.

Fix

My naive fix would be to make a copy of result.queryset in the __deepcopy__ method, like so:

result.queryset = result.queryset.all()

But my understanding of the issue is limited and I probably don't know the ins and outs of this (especially considering the lengthy discussion around ticket #11183).

Change History (5)

comment:1 by Spark23, 12 years ago

Owner: changed from nobody to Spark23
Status: newassigned

comment:2 by Karen Tracey, 12 years ago

Note this commit: https://github.com/django/django/commit/ee96c7eb2d

rather sounds like it intended to do what is being asked for here, which is to ensure that the queryset associated with a model choice field be re-evaluated when necessary, and not stuck at whatever happened to be the choices associated with the queryset when the form was initially defined. I'm not entirely sure if code has changed in the interim such that the behavior has broken (the commit has associated tests so I would hope that has not happened) or if that commit didn't entirely cover the need here).

comment:3 by Preston Holmes, 12 years ago

This seems like a pretty far edge case. The internals already clear the cache as pointed out - specifically here:

https://github.com/django/django/commit/ee96c7eb2d#L0R120

while altering forms in init is a pretty common pattern, I don't think this would be a regularly encountered problem - though granted, I'm sure it would stump more than a few.

however calling .all() to get a copy is not 'free', and clearing the cache is a more precise way to avoid the problem for the implementation. I think that if you are going to get into this level of form override, your workaround is the correct way to deal with it - not to clone the queryset in deepcopy

I'd suggest this is a candidate for "wontfix"

comment:4 by Melvyn Sopacua, 12 years ago

Triage Stage: UnreviewedDesign decision needed

As per ptone's comments, it is a confirmed issue, but edge case. The work around may need some documentation.

comment:5 by Florian Apolloner, 12 years ago

Resolution: wontfix
Status: assignedclosed

Closing as wontfix as suggested by Preston.

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