yourlabs/django-autocomplete-light

Django Widget protocol not implemented properly in WidgetMixin

ercpe opened this issue · 2 comments

ercpe commented

Today we hit a nasty bug with dal.ModelSelect2 fields where an autocomplete field would receive forwards from a different class which uses the same base class.

We traced the error back to the WidgetMixin where we found that it does not implement __deepcopy__ properly and thus would copy a reference to the forward list between instances of the widget.

Background: When Django creates a Form instance from a form class, the fields and widgets are copied instead of instantiated. This means that every field/widget must implement __deepcopy__ properly to create clones of any references it holds.

Example to reproduce:

        class MyBaseForm(forms.Form):
            choice_field = forms.ModelChoiceField(User.objects.all(),
                                                  widget=autocomplete.ModelSelect2(url='/somewhere'))

        class Form1(MyBaseForm):
            def __init__(self, *args, **kwargs):
                super().__init__(*args, **kwargs)
                self.fields['choice_field'].widget.forward.append(forward.Const(True, 'filter_a'))

        class Form2(MyBaseForm):
            def __init__(self, *args, **kwargs):
                super().__init__(*args, **kwargs)
                self.fields['choice_field'].widget.forward.append(forward.Const(True, 'filter_b'))


        form1 = Form1()
        print([x.__dict__ for x in form1.fields['choice_field'].widget.forward]) # prints only filter_a

        form2 = Form2()
        print([x.__dict__ for x in form2.fields['choice_field'].widget.forward]) # prints filter_a and filter_b

In this example, the reference to the forward list is copied once when the Form1 instance is created and again when the Form2 instance is created. Thus, the widget.forward.append call in Form2 adds the forward to the very same list and now affects every instance of the form (in the same python process).

To fix the issue:

Implement __deepcopy__ in WidgetMixin and copy() the forward, e.g.

    def __deepcopy__(self, memo):
        clone = super().__deepcopy__(memo)
        clone.forward = self.forward.copy()
        return clone

See django.forms.widgets.Widget.__deepcopy__ and django.forms.widgets.ChoiceWidget.__deepcopy__ for an example in Django itself where mutable types are copied when the Widget is cloned.

Also, the forward argument to the WidgetMixin.__init__ should also be copied as it has the same issue.

jpic commented

Interesting, let's see what a pull request looks like with this