Opened 6 years ago

Closed 5 years ago

#16138 closed Bug (fixed)

ModelForm's 'initial' attribute unexpectedly persists across different instances

Reported by: Haisheng HU Owned by: nobody
Component: Generic views Version: 1.3
Severity: Normal Keywords: model form, generic view
Cc: hanson2010@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

There are two UpdateView's (or CreateView's) A and B with their corresponding ModelForm A and B. From UpdateView A the 'initial' attribute in ModelForm A was set to a certain value. After this, UpdateView B and ModelForm B were created. Now the 'initial' attribute in ModelForm B was not {}, but the same as ModelForm A.

class UpdateViewA(UpdateView):
    model = Project
    form_class = ProjectFormA
    ...
    def get_form_kwargs(self, **kwargs):
        kwargs = super(UpdateViewA, self).get_form_kwargs(**kwargs)
        if self.get_object().kickoff_date == None:
            kwargs['initial']['kickoff_date'] = datetime.date.today()
        return kwargs

class UpdateViewB(UpdateView):
    model = Project
    form_class = ProjectFormB
    ...

class ProjectFormA(ModelForm):
    ...
    class Meta:
        model = Project

class ProjectFormB(ModelForm):
    ...
    class Meta:
        model = Project

Attachments (4)

reference_form_initial's_shadow.diff (427 bytes) - added by Haisheng HU 6 years ago.
form_mixin_test.diff (1.8 KB) - added by wilfred@… 5 years ago.
Test case to ensure get_initial() returns a fresh dict
formmixin_change_documented.diff (605 bytes) - added by wilfred@… 5 years ago.
documenting behaviour change in the docs
16138-4.diff (3.5 KB) - added by Claude Paroz 5 years ago.
Consolidate fix/test/docs patches

Download all attachments as: .zip

Change History (16)

Changed 6 years ago by Haisheng HU

comment:1 Changed 6 years ago by Haisheng HU

Cc: hanson2010@… added
Keywords: model form generic view added
Summary: ModelForm's 'initial' attribute expectedly persists cross different instancesModelForm's 'initial' attribute expectedly persists across different instances

comment:2 Changed 6 years ago by Aymeric Augustin

Needs tests: set
Triage Stage: UnreviewedAccepted

One could expect that the accessor get_initial() return an object value that can be safely mutated. Your patch achieves this.

The alternative is to forbid mutating the result of get_initial() in the documentation. You would have to override get_initial() instead of get_form_kwargs().

    def get_initial(self):
        result = {}
        if self.get_object().kickoff_date == None:
            result['kickoff_date'] = datetime.date.today()
        result.update(super(..., self).get_initial())
        return result

I'm not sufficiently familiar with the generic view to make a decision here, but either the code or the docs must be improved.

comment:3 Changed 6 years ago by Haisheng HU

Summary: ModelForm's 'initial' attribute expectedly persists across different instancesModelForm's 'initial' attribute unexpectedly persists across different instances

The alternative solution raised by @aaugustin is feasible, although it's not so straightforward to construct a local dict and update it with a super class method.

Some people adopted the get_form_kwargs way:
http://stackoverflow.com/questions/4512254/user-current-user-on-new-django-class-views-forms

Just my 2 cents.

comment:4 Changed 5 years ago by Gabriel Hurley

Needs documentation: set
UI/UX: unset

get_initial shouldn't return an object that's unsafe to mutate, and I can't see a compelling reason for it to return an immutable object. The greatest flexibility would be afforded by a method as in the patch since it allows you to alter self.initial if you so desire, but also prevents the gotcha case of accidentally mutating the initial data for the entire process.

This patch still needs a test case, and a note in the documentation would also be helpful.

comment:5 Changed 5 years ago by Aymeric Augustin

#16701 was a duplicate.

Changed 5 years ago by wilfred@…

Attachment: form_mixin_test.diff added

Test case to ensure get_initial() returns a fresh dict

comment:6 Changed 5 years ago by Florian Apolloner

milestone: 1.4

comment:7 Changed 5 years ago by Jacob

milestone: 1.4

Milestone 1.4 deleted

Changed 5 years ago by wilfred@…

documenting behaviour change in the docs

comment:8 Changed 5 years ago by wilfred@…

Needs documentation: unset

Documentation added. I've stated '1.4' in my docs patch, although I was unsure. Should it say 'trunk' instead?

comment:9 Changed 5 years ago by Claude Paroz

#17759 was a duplicate (ans has a patch, too).

Changed 5 years ago by Claude Paroz

Attachment: 16138-4.diff added

Consolidate fix/test/docs patches

comment:10 Changed 5 years ago by Claude Paroz

Needs tests: unset

comment:11 Changed 5 years ago by Jannis Leidel

Triage Stage: AcceptedReady for checkin

comment:12 Changed 5 years ago by Claude Paroz

Resolution: fixed
Status: newclosed

In [17765]:

Fixed #16138 -- Made FormMixin get_initial return a copy of the 'initial' class variable. Thanks hanson2010, wilfred@… and agriffis for their work on the patch.

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