Description

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

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:

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.

Bug - Model.full_clean() not used anywhere in Django? →
Model docs imply that ModelForm will call Model.full_clean(), but it won't.

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.

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

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.

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.

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.

@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:

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