Opened 8 years ago

Closed 6 years ago

#15587 closed Bug (duplicate)

django.forms.BaseModelForm._post_clean updates instance even when form validation fails

Reported by: lanyjie Owned by: nobody
Component: Forms Version: 1.3
Severity: Normal Keywords: data validation
Cc: vmagamedov@…, Sergey Maranchuk Triage Stage: Accepted
Has patch: no Needs documentation: yes
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no


This method will update the model instance with self.cleaned_data.

    self.instance = construct_instance(self, self.instance, opts.fields, opts.exclude)

however, the cleaned_data might not be good. It is probably good to have a single line inserted before the first line of code of this method:

def _post_clean(self):
   if self._errors: return

Change History (7)

comment:1 Changed 8 years ago by Luke Plant

Resolution: needsinfo
Severity: Normal
Status: newclosed
Type: Uncategorized

I'm afraid this bug report is very unclear - please give an example that shows an actual bug and re-open. Thanks!

comment:2 Changed 8 years ago by Chris Beaven

Type: UncategorizedBug

comment:3 Changed 8 years ago by Vladimir Magamedov

Cc: vmagamedov@… added
Easy pickings: unset
Has patch: unset
Resolution: needsinfo
Status: closedreopened
UI/UX: unset
Version: 1.21.3

I have the same problem with model forms:

>>> from django.contrib.auth.models import User
>>> from django.contrib.auth.forms import UserChangeForm
>>> user = User.objects.all()[0]
>>> form = UserChangeForm({'first_name': 'Vladimir', 'email': 'invalid'}, instance=user)
>>> user.first_name
>>> form.is_valid()
>>> user.first_name

As you can see, this form changes provided instance after validation. This behavior is not expected and I'm considering this as a bug.

Or this is not a bug? Maybe this is a design decision to make it possible to validate form data in the BaseModel.clean_fields method? This is odd for me.

Provided single-line patch in the ticket description isn't taking into account that validation error can be raised later by the model validation, so this is wouldn't fix this problem.

comment:4 Changed 8 years ago by Sergey Maranchuk

Cc: Sergey Maranchuk added

comment:5 Changed 8 years ago by Bas Peschier

Needs documentation: set
Summary: problem in django.forms.BaseModelForm._post_cleandjango.forms.BaseModelForm._post_clean updates instance even when form validation fails
Triage Stage: UnreviewedAccepted

The question here is at which point the form is allowed to update the instance. Other tickets are also having problems here for their own reasons (#15995, #16423).

There is a strong argument for doing this while validating since ModelForm actually promises to fully validate the data (form and model). For this the instance needs to be updated.

However, there is case for reminding people that in current workflow validation implies updating the model to independently validate the model; so if anybody can create a patch for the docs explaining this.

Otherwise this will become a DDN and we need to discuss whether a form validation error means the field should not be updated on the model.

comment:6 Changed 6 years ago by Aymeric Augustin

Status: reopenednew

comment:7 Changed 6 years ago by Tim Graham

Resolution: duplicate
Status: newclosed

Duplicate of #14885 which was fixed by a documentation update.

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