Code

Opened 4 years ago

Closed 4 years ago

Last modified 3 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 russellm)

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 4 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 4 years ago by russellm

  • Description modified (diff)
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to invalid
  • Status changed from new to closed

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 4 years ago by jkocherhans

  • Resolution invalid deleted
  • Status changed from closed to reopened
  • Triage Stage changed from Unreviewed to Accepted

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 4 years ago by SmileyChris

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 follow-up: Changed 4 years ago by patrys

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

comment:5 in reply to: ↑ 4 ; follow-up: Changed 4 years ago by jkocherhans

  • Has patch set
  • Owner changed from nobody to jkocherhans
  • Patch needs improvement set
  • Status changed from reopened to new

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 ; follow-up: Changed 4 years ago by patrys

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 4 years ago by jkocherhans

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 4 years ago by jkocherhans

  • Status changed from new to assigned

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 4 years ago by jkocherhans

  • Resolution set to fixed
  • Status changed from assigned to closed

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

comment:9 Changed 3 years ago by jacob

  • milestone 1.2 deleted

Milestone 1.2 deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.