Model docs imply that ModelForm will call Model.full_clean(), but it won't.

[Oroku Saki]

I created a documentation ticket for this thinking the model validation docs weren't clear enough, but then on further investigation, here's what I found (A pretty important bug for those using the new model validation):

BaseForm.is_valid() calls BaseForm._get_errors() calls BaseForm.full_clean() calls BaseForm._post_clean() which calls the following 3 methods:

Model.clean_fields()
Model.clean()
BaseForm.validate_unique() which calls Model.validate_unique()

Model.full_clean() as defined on line 808 of django.db.models.base.py is never called anywhere in Django, period. This means that the docs are incorrect because ModelForms do not include a call to Model.full_clean(). This means that you cannot use transactions in Model methods (except maybe with hacking) because a full validation from one method isn't included. I'm trying to do this:

    @transaction.commit_on_success
    def full_clean(exclude=None):
        super(MyModel, self).full_clean(exclude)
    
    def clean(self):
        # Creating necessary related instance to attach to self.some_foreign_key (expecting this to be undone buy transaction mentioned above if self.save() never completes)

But this is not possible because my full_clean() method is never called unless I create a view for this and manually run my_new_instance.full_clean(), but then it defeats the purpose of doing it, which is to protect my model's integrity at the model level so that I can use the Admin, a custom view, or a JSON API using Piston.

[Oroku Saki]

Oops, forgot to categorize it.

[jkocherhans]

Model.full_clean() is never called anywhere in Django. This is by design. ModelForm.full_clean() cannot call Model.full_clean() and still be backwards compatible.

The specific behavior of Model.full_clean() is documented in the 3rd paragraph ​here. It says: "Note that full_clean() will NOT be called automatically when you call your model’s save() method. You’ll need to call it manually if you want to run model validation outside of a ModelForm. (This is for backwards compatibility.)"

I can see how that could be read as saying that ModelForm will call Model.full_clean() and should be clarified. I'm inclined to just pull "if you want to run model validation outside of a ModelForm". Documenting the specific behavior would just be documenting implementation details. What's really going on is that we're trying to mimic the behavior of ModelForm from 1.1. That doesn't make for very good documentation though.

I think model-validation is just not what you want it to be. It's a step on the way there, but we can't start completely validating models before we save them without an extended deprecation plan for the current non-validating behavior. All of the authors and people familiar with the implementation are busy until after 1.2 is out, but we should have a conversation on django-dev a week or so after that happens.

If you want full model validation, you'll have to call Model.full_clean() yourself. That much is documented.

[jkocherhans]

See #13097 for more places in the documentation regarding this issue that might need some love.

[Oroku Saki]

ModelForm can call Model.full_clean() without breaking backwards compatibility.

It can't happen on Model.save() but rather on BaseModelForm._post_clean(). This would cause the exact same behavior, but would allow for transactions on your model's built in methods (like my reimplementation of Model.full_clean() above).

Here's the current BaseModelForm._post_clean()

    def _post_clean(self):
        exclude = self._get_validation_exclusions()
        opts = self._meta

        # Update the model instance with self.cleaned_data.
        self.instance = construct_instance(self, self.instance, opts.fields, opts.exclude)

        # Clean the model instance's fields.
        try:
            self.instance.clean_fields(exclude=exclude)
        except ValidationError, e:
            self._update_errors(e.message_dict)

        # Call the model instance's clean method.
        try:
            self.instance.clean()
        except ValidationError, e:
            self._update_errors({NON_FIELD_ERRORS: e.messages})

        # Validate uniqueness if needed.
        if self._validate_unique:
            self.validate_unique()

Here's my new version:

    def _post_clean(self):
        exclude = self._get_validation_exclusions()
        opts = self._meta

        # Update the model instance with self.cleaned_data.
        self.instance = construct_instance(self, self.instance, opts.fields, opts.exclude)

        # Perform all model validation
        try:
            self.instance.full_clean(exclude=exclude)
        except ValidationError, e:
            self._update_errors(e.message_dict)

[Oroku Saki]

I have a suggestion, and I'm willing to get involved (at a deeper level than just griping like I currently do) with some guidance. Here's what I suggest:

1) Add validators to each django.db.models.fields.Field subclass (based on type like EmailField, etc) using the new validators plug-in architecture. (some already exist like EmailValidator)
2) This doesn't effect existing models API one bit as Model.clean_fields() (which is not called except for in Models.full_clean()) is the only place where these validators are used.
3) Make a new version of ModelForm called NewModelForm :) that is almost identical, except that illuminates all form level validation (simple to do), and only uses propagated validation from the model which it is bound to.
5) Leave Model.full_clean() as is, but add call to it into NewModelForm.full_clean(). Now, only use only the newly propagated validation errors (remove the self._clean_fields() self._clean_form() self._post_clean()) from line 266+ on django.forms.forms.py (or leave it so that you can have model validation plus extra hooked in form validation).

Now we have exactly the same API with one backwards-compatible addition: NewModelForm. This might sound like a joke, but think about how successful NewForms were :)

This is all very quick and easy... except for porting all the validators into each model field. As I said, I will help, but only if it will be added and supported. I don't want to have a branch.

[Oroku Saki]

Please delete above post. Sorry for not reading about WikiFormatting first. Here it is reformatted:

I have a suggestion, and I'm willing to get involved (at a deeper level than just griping like I currently do) with some guidance. Here's what I suggest:

1) Add validators to each django.db.models.fields.Field subclass (based on type like EmailField, etc) using the new validators plug-in architecture. (some already exist like EmailValidator)

2) This doesn't effect existing models API one bit as Model.clean_fields() (which is not called except for in Models.full_clean()) is the only place where these validators are used.

3) Make a new version of ModelForm called NewModelForm :) that is almost identical, except that illuminates all form level validation (simple to do), and only uses propagated validation from the model which it is bound to.

4) Leave Model.full_clean() as is, but add call to it into NewModelForm.full_clean(). Now, only use only the newly propagated validation errors (remove the self._clean_fields() self._clean_form() self._post_clean()) from line 266+ on django.forms.forms.py (or leave it so that you can have model validation plus extra hooked in form validation).

Now we have exactly the same API with one backwards-compatible addition: NewModelForm. This might sound like a joke, but think about how successful NewForms were :)

This is all very quick and easy... except for porting all the validators into each model field. As I said, I will help, but only if it will be added and supported. I don't want to have a branch.

[Russell Keith-Magee]

I'm going to side with Joseph on this one -- he and Honza spent a long time developing model validation, and I have a lot of confidence in his design decisions.

On the other hand, your solution appears to hang on the idea of introducing a new ModelForm class. The analog with newforms doesn't apply, because we deleted oldforms. We can't delete ModelForm, and I have no interest in attempting to maintain parallel implementations of the same feature.

Marking accepted because Joseph has accepted the documentation issue.

[Oroku Saki]

@russellm I don't think you even need to maintain 2 implementations. Did you check out my "New Version" above where I simply called full_clean() instead of using full_clean()'s implementation? Here's what I mean:

Here is ModelForm._post_clean():

    def _post_clean(self):
        exclude = self._get_validation_exclusions()
        opts = self._meta

        # Update the model instance with self.cleaned_data.
        self.instance = construct_instance(self, self.instance, opts.fields, opts.exclude)

        # Clean the model instance's fields.
        try:
            self.instance.clean_fields(exclude=exclude)
        except ValidationError, e:
            self._update_errors(e.message_dict)

        # Call the model instance's clean method.
        try:
            self.instance.clean()
        except ValidationError, e:
            self._update_errors({NON_FIELD_ERRORS: e.messages})

        # Validate uniqueness if needed.
        if self._validate_unique:
            self.validate_unique()

And here is Model.full_clean():

    def full_clean(self, exclude=None):
        """
        Calls clean_fields, clean, and validate_unique, on the model,
        and raises a ``ValidationError`` for any errors that occured.
        """
        errors = {}
        if exclude is None:
            exclude = []

        try:
            self.clean_fields(exclude=exclude)
        except ValidationError, e:
            errors = e.update_error_dict(errors)

        # Form.clean() is run even if other validation fails, so do the
        # same with Model.clean() for consistency.
        try:
            self.clean()
        except ValidationError, e:
            errors = e.update_error_dict(errors)

        # Run unique checks, but only for fields that passed validation.
        for name in errors.keys():
            if name != NON_FIELD_ERRORS and name not in exclude:
                exclude.append(name)
        try:
            self.validate_unique(exclude=exclude)
        except ValidationError, e:
            errors = e.update_error_dict(errors)

        if errors:
            raise ValidationError(errors)

ModelForm._post_clean() is simply repeating the exact implementation of Model.full_clean(), instead of just calling Model.full_clean(). It's not a bug, but is not a good idea to repeat the implementation of an abstraction and very much breaks DRY. The only difference between its implementation and ModelForm._post_clean() is the internal check it makes before running validate_unique().

The benefit it adds is that you can then override full_clean() to use a database transaction. This is absolutely necessary to do what Django recommends (model altering inside of clean()).

Pretty please change this.

Again, here is my patch:

ModelForm._post_clean():

    def _post_clean(self):
        exclude = self._get_validation_exclusions()
        opts = self._meta

        # Update the model instance with self.cleaned_data.
        self.instance = construct_instance(self, self.instance, opts.fields, opts.exclude)

        # Perform all model validation by using the already implemented Model.full_clean() (which saves time by not having 2 implementations of almost the exact same code)
        try:
            self.instance.full_clean(exclude=exclude)
        except ValidationError, e:
            self._update_errors(e.message_dict)

Again, I will test it and perform all steps along the way if this is accepted.

[Preston Holmes]

Improvements to model validation docs

[Preston Holmes]

durr - trac needs the diff extension to make it perty

[Preston Holmes]

The docs are wrong in that they state that modelform calls model.full_clean directly in the second sentence where it says: " Most of the time, this method will be called automatically by a ModelForm."

I've submitted a patch which I think clears up this part of the docs.

[Russell Keith-Magee]

(In [13160]) Fixed #13100 -- Clarified the model validation rules around full_clean(). Thanks to ptone for the draft text.

[Jacob]

Milestone 1.2 deleted