Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Django Widget protocol not implemented properly in WidgetMixin #1333

Open
ercpe opened this issue Jul 5, 2023 · 2 comments
Open

Django Widget protocol not implemented properly in WidgetMixin #1333

ercpe opened this issue Jul 5, 2023 · 2 comments

Comments

@ercpe
Copy link

ercpe commented Jul 5, 2023

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
Copy link
Member

jpic commented Jan 9, 2024

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

ercpe added a commit to ercpe/django-autocomplete-light that referenced this issue Mar 23, 2024
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.

Implement `__deepcopy__` in the `WidgetMixin` and copy the `forward` list to prevent forwards from leaking into unrelated forms when using a common base class.
@ercpe
Copy link
Author

ercpe commented Mar 23, 2024

I finally got around to create a Merge Request with the changes we made to a subclass in our project: #1358

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants