Opened 15 years ago

Closed 15 years ago

Last modified 13 years ago

#11183 closed (fixed)

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

Reported by: Margie Roginski Owned by: Luke Plant
Component: Forms Version: 1.0
Severity: Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Luke Plant)

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.

Attachments (2)

11183 (1.2 KB ) - added by Margie Roginski 15 years ago.
11183.2.diff (1.8 KB ) - added by Luke Plant 15 years ago.
revised patch

Download all attachments as: .zip

Change History (14)

by Margie Roginski, 15 years ago

Attachment: 11183 added

comment:1 by Luke Plant, 15 years ago

Description: modified (diff)
Needs tests: set

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

comment:2 by Thejaswi Puthraya, 15 years ago

Component: UncategorizedForms

comment:3 by Russell Keith-Magee, 15 years ago

milestone: 1.2
Triage Stage: UnreviewedAccepted

comment:4 by Luke Plant, 15 years ago

Owner: changed from nobody to Luke Plant
Status: newassigned

by Luke Plant, 15 years ago

Attachment: 11183.2.diff added

revised patch

comment:5 by Luke Plant, 15 years ago

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

comment:6 by Alex Gaynor, 15 years ago

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

comment:7 by Margie Roginski, 15 years ago

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

comment:8 by Luke Plant, 15 years ago

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.

comment:9 by Luke Plant, 15 years ago

Resolution: fixed
Status: assignedclosed

Fixed in [12733]

comment:10 by Luke Plant, 15 years ago

(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

comment:11 by Luke Plant, 15 years ago

For the record, in reply to Alex:

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

comment:12 by Jacob, 13 years ago

milestone: 1.2

Milestone 1.2 deleted

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