Opened 13 years ago

Closed 11 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

Description

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 by Luke Plant, 13 years ago

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 by Chris Beaven, 13 years ago

Type: UncategorizedBug

comment:3 by Vladimir Magamedov, 13 years ago

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
u''
>>> form.is_valid()
False
>>> user.first_name
u'Vladimir'

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 by Sergey Maranchuk, 13 years ago

Cc: Sergey Maranchuk added

comment:5 by Bas Peschier, 13 years ago

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 by Aymeric Augustin, 11 years ago

Status: reopenednew

comment:7 by Tim Graham, 11 years ago

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