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 Version 1
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 (last modified by )
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 thatWidget::__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 (2)
by , 16 years ago
comment:1 by , 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).