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

[krejcik]

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)

[Russell Keith-Magee]

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).

[jkocherhans]

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.

[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).

[Patryk Zawadzki]

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

[jkocherhans]

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.

[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.

[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.

[jkocherhans]

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.

[jkocherhans]

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

[Jacob]

Milestone 1.2 deleted