Opened 7 years ago

Closed 7 years ago

Last modified 5 years ago

#12960 closed (fixed)

return value (cleaned_data) from clean() method is ignored

Reported by: krejcik Owned by: jkocherhans
Component: Forms Version: master
Severity: Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: UI/UX:

Description (last modified by Russell Keith-Magee)

Any changes made by model's clean method are ignored.

Sequence of calls in BaseForm.full_clean() is following:

  1. _clean_fields - which calls clean on fields and creates instance with cleaned values
  2. _clean_form - which class clean on model, assign return to form.cleaned_data but instance is not updated
  3. finally save_instance(construct=False) is called (in previous version of Django model instance was created here from correct cleaned_data)

Attachments (1)

12960.diff (4.7 KB) - added by jkocherhans 7 years ago.
Complete patch. Still thinking about it, but this moves model validation to happen entirely after form validation. I think this is a good thing, but I'm not entirely sure yet.

Download all attachments as: .zip

Change History (10)

comment:1 Changed 7 years ago by Russell Keith-Magee

Description: modified (diff)
Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Resolution: invalid
Status: newclosed

Closing invalid because I can't understand what problem you are describing. You seem to be describing some sort of regression with cleaned_data handling, but then you talk about the clean method on a *model*, which suggests you mean model validation.

An example would be extremely helpful. Please reopen if you care to provide sample code, along with usage, and a description of what is happening, what should be happening (and what used to happen if this is actually a regression).

comment:2 Changed 7 years ago by jkocherhans

Resolution: invalid
Status: closedreopened
Triage Stage: UnreviewedAccepted

Hey Russ. I actually looked in to this yesterday, but forgot to mark it as accepted. The problem is that the return value of ModelForm.clean() will never be applied to the model.

comment:3 Changed 7 years ago by Chris Beaven

Here's a quick review the current situation:

  • clean form fields
  • clean model fields
  • clean model
  • clean form

The model is cleaned before the form so the model's clean method gets a chance to fix fields before the form's validate_unique method is run.

This puts us into a cyclic problem in where we need to check the model instance before actually being able to clean the form, which may alter the form's cleaned_data. But we need the cleaned_data to accurately build the instance. And we also aren't allowed to call model field's save_form_data more than once (according to other tests).

comment:4 Changed 7 years ago by Patryk Zawadzki

Could we fix it by (re)constructing the instance at the end of full_clean()?

comment:5 in reply to:  4 ; Changed 7 years ago by jkocherhans

Has patch: set
Owner: changed from nobody to jkocherhans
Patch needs improvement: set
Status: reopenednew

Replying to patrys:

Could we fix it by (re)constructing the instance at the end of full_clean()?

That's part of a possible solution, but it isn't enough on its own. Model.clean_fields() and Model.clean() can modify the model instance in place, so applying all of form.cleaned_data to the model after that happens could cause different problems.

comment:6 in reply to:  5 ; Changed 7 years ago by Patryk Zawadzki

Replying to jkocherhans:

That's part of a possible solution, but it isn't enough on its own. Model.clean_fields() and Model.clean() can modify the model instance in place, so applying all of form.cleaned_data to the model after that happens could cause different problems.

It shouldn't be possible to do the same thing by either:

  • touching self.instance
  • or touching self.cleaned_data
  • or returning a modified copy of self.cleaned_data

I think self.instance existing at the point when clean() is called is an implementation detail and is not guaranteed by the API documentation. Modifying the instance directly inside clean() should be discouraged.

Changed 7 years ago by jkocherhans

Attachment: 12960.diff added

Complete patch. Still thinking about it, but this moves model validation to happen entirely after form validation. I think this is a good thing, but I'm not entirely sure yet.

comment:7 in reply to:  6 Changed 7 years ago by jkocherhans

Status: newassigned

Replying to patrys:

I think self.instance existing at the point when clean() is called is an implementation detail and is not guaranteed by the API documentation. Modifying the instance directly inside clean() should be discouraged.

I don't think we can discourage modifying the instance inside Model.clean(), but for ModelForm.clean() you're absolutely right. I think my latest patch hits the mark. Instance creation is postponed until after the form validation has completed, then the model validation is run, then unique_validation is run as long as ModelForm.clean() wasn't overridden without calling super.

Short version:

  • clean form fields
  • run form.clean()
  • create or update form.instance with form.cleaned_data
  • clean model fields
  • run instance.clean()
  • optionally run form.validate_unique()

This seems a lot easier to reason about to me. I'd still like Honza to check this out, but I feel pretty good about it at this point. Hopefully I'm not missing some corner case.

comment:8 Changed 7 years ago by jkocherhans

Resolution: fixed
Status: assignedclosed

(In [12690]) Fixed #12960. The return value of ModelForm.clean() is now applied to the model. Thanks for the report, krejcik.

comment:9 Changed 5 years ago by Jacob

milestone: 1.2

Milestone 1.2 deleted

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