Opened 16 years ago

Last modified 13 years ago

#11183 closed

BaseForm init leaves pointers pointing back to base_fields when widget is a Select — at Initial Version

Reported by: Margie Roginski Owned by: nobody
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

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

Change History (1)

by Margie Roginski, 16 years ago

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