Opened 4 years ago

Closed 2 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@…, slav0nic 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 Changed 4 years ago by lukeplant

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to needsinfo
  • Severity set to Normal
  • Status changed from new to closed
  • Type set to 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 4 years ago by SmileyChris

  • Type changed from Uncategorized to Bug

comment:3 Changed 4 years ago by vmagamedov

  • Cc vmagamedov@… added
  • Easy pickings unset
  • Has patch unset
  • Resolution needsinfo deleted
  • Status changed from closed to reopened
  • UI/UX unset
  • Version changed from 1.2 to 1.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 Changed 4 years ago by slav0nic

  • Cc slav0nic added

comment:5 Changed 4 years ago by bpeschier

  • Needs documentation set
  • Summary changed from problem in django.forms.BaseModelForm._post_clean to django.forms.BaseModelForm._post_clean updates instance even when form validation fails
  • Triage Stage changed from Unreviewed to Accepted

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 2 years ago by aaugustin

  • Status changed from reopened to new

comment:7 Changed 2 years ago by timo

  • Resolution set to duplicate
  • Status changed from new to closed

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

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