BaseForm init leaves pointers pointing back to base_fields when widget is a Select

[Margie Roginski]

High Level Description

When we create a form class, I think the deepcopy that is done does not do enough to correctly copy the class in the case when the widget is a Select widget. The result is that parts of the class are left pointing back to the base_fields data, and when one tries to override the queryset at during render(), it is not possible due pointers that are incorrectly pointing back to base_fields data.

Detail

When a base_field in a form points to a Select widget, the widget contains a choices attribute. During initialiazation of a ModelChoiceField, a ModelChoiceIterator is created and the widget's choices attribute is set to point to it. This is all done by the following line from ModelChoiceField::__init__():

        self.queryset = queryset

When BaseForm:__init__() runs, it does a deepcopy of self.base_fields. When copying the fields, we call the Widget::__deepcopy__() method. However, this method copies only the attributes and does not copy the ModelChoiceIterator that a select widget points to. I think that a copy should be made of this ModelChoiceIterator, and furthermore, I think that after making the copy, the newly copied ModelChoiceIterator's 'field' attribute needs to be updated so that it points to the copied field whose deepcopy caused it to be copied.

I encountered this issue when I tried to override the queryset for a ModelChoiceField with a line like this in the render() function of a widget that derives from a Select widget. Here is some example code that my usage:

class OwnerSelectWidget(widgets.Select):
    def __init__(self, rel, attrs=None):
        self.rel = rel
        super(OwnerSelectWidget, self).__init__(attrs)


    def render(self, name, value, attrs=None):
        key = self.rel.get_related_field().name
        self.choices.field.queryset = self.rel.to._default_manager.filter(**{key: value})  <<=== important
        rendered = super(OwnerSelectWidget, self).render(name, value, attrs)

When the self.choices.field.queryset assignment is made, it references through the ModelChoiceIterator to the field associated with the widget. In the current code base, at this point int the code, the widget has been copied, but it still references the original ModelChoiceIterator, which references the ModelChoiceField that is in base_fields, which references the original "base_fields" widget. This is different than the 'self' widget, which is the copied version. The result is that the self.choices.field.queryset assignment updates the wrong widget (the one from base_fields) with the new queryset.

I made a patch to my code that seems to work. However, I find the patch kind of ugly and I suspect someone that understands the code better than me might want to do this a different way. I did two things:

• Created a __deepcopy__() method for the Select widget, which does the same thin that Widget::__deepcopy__() does, but also calls copy on the self.choices field
• Created a small loop that follows the deepcopy in BaseForm:__init__(), which runs through all fields that were copied, and if the field contains a'widget' attribute and that widget attribute contains a 'choices' attribute and that 'choices' attribute contains a 'field' attribute, then it sets that 'field' attribute point to the newly copied field. This is the part that I think is pretty ugly.

The attached svn diff shows the patch.

I ran the django testsuite with the command ./runtests.py --settings=settings and all tests passed and I have verified via my own debugging that this solves my problem.

[Luke Plant]

Thanks for your work debugging this. If you could create a regression test that corners the problem, that would be great. Put it in 'tests/regressiontests/forms/regressions.py' (unless you can find a better place for it).

[Luke Plant]

revised patch

[Luke Plant]

margieroginski - I came up with a slightly different patch. Could you check that my patch fixes this bug in the context of your problem?

[Alex Gaynor]

The __deepcopy__ methods on Widgets/Fields seem to be becoming a nest full of random special casing for the different attributes different ones have. I can't figure out why, for example, Field even bothers to define this deepcopy, the default implementation should suffice: ​http://code.djangoproject.com/browser/django/trunk/django/forms/fields.py#L167

[Margie Roginski]

Hi Luke,

I unfortunately am not using this code or functionality anymore. Early on in my django work I was trying to use the admin app as my end user app, but I have long since abandoned that. It seems to me that in general choices does not get updated on the widget associated with fields that use choices. For example, I think that if I create a ChoiceField in my form, and then recreate the ChoiceField with different choices, the underlying widget will not get those choices. I believe I ran into this and Malcolm said this was the case, though I am a bit hazy on it now.

In any case, I have not looked at the above code in almost a year since it is no longer an issue for me, so I don't think I can add a lot of value on it at this point. Sorry about that ...

Margie

[Luke Plant]

Thanks for responding Margie.

It seems to me that the bug cornered in my patch is a genuine bug. I don't know about the __deepcopy__ issues that Alex brought up, but I'll commit this fix for now, with the hope of finding/explaining the real problem in the future.

[Luke Plant]

Fixed in [12733]

[Luke Plant]

(In [12736]) [1.1.X] Fixed #11183 - BaseForm init leaves pointers pointing back to base_fields

Thanks to margieroginski for the report

Backport of [12733] from trunk

[Luke Plant]

For the record, in reply to Alex:

• There is a lot of discussion on this on #5505
• relevant commits are [6156], [6415], [6450]

The current state is that if I remove the __deepcopy__ from Widget and subclasses I don't actually get any failing tests, but there may not be any specific tests for the problems of not defining that method. If I remove the __deepcopy__ from Field and subclasses I get lots of failing tests and exceptions (mainly seem to be about copying regexes).

[Jacob]

Milestone 1.2 deleted