#15323 closed (invalid)
django.db.models.Model.clean() should be independent from .clean_fields()
Reported by: | Tim Molendijk | Owned by: | nobody |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
Severity: | Keywords: | model validation clean | |
Cc: | Triage Stage: | Unreviewed | |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
From http://code.djangoproject.com/browser/django/trunk/django/db/models/base.py#L811:
# Form.clean() is run even if other validation fails, so do the # same with Model.clean() for consistency.
This behavior can be surprisingly annoying. Assume a model Car
with a foreign key field (null=False
) named owner
. Now assume one of the restrictions on Car
is that owner.gender == 'm'
(yeah ridiculous, I know ;)). One would think that a model clean method as follows would do the job:
def clean(self): if self.owner.gender != 'm': raise ValidationError("Women are not allowed to own cars!")
BUT, this will crash the system every time you try to save a car instance that has no owner assigned. One would say that clean()
does not have to worry about the requirement that an owner is obligatory, because that's what clean_fields()
is for, right? WRONG, because clean()
will always run, regardless of the success of clean_fields()
. And running clean()
will result in an ObjectDoesNotExist
error because self.owner
does not have a value.
The consequence is that clean()
must anticipate on all potential validation errors in clean_fields()
. Bye bye separation of concerns...
Change History (3)
comment:1 by , 14 years ago
Resolution: | → invalid |
---|---|
Status: | new → closed |
follow-up: 3 comment:2 by , 14 years ago
Yeah, hear what you're saying. Backward compatibility is good, but for every price? Too many of these relatively minor design flaws == death by 1000 papercuts
comment:3 by , 14 years ago
Replying to timmolendijk:
Yeah, hear what you're saying. Backward compatibility is good, but for every price? Too many of these relatively minor design flaws == death by 1000 papercuts
Consider a form with three required values A, B, C. Additionally, it checks in clean
(that's the only place you can do multi-field validation, right?) that A != B.
The user posts: A=5, B=5 and C is missing. So clean_C
will fail with "required" error. If clean
doesn't trigger, the user won't see the error concerning A beign equal to B. IMHO, this is wasteful and bad from UX point of view.
The same applies to validation in models. Providing more validation is better and the current API lets you do that.
Like the comment says, this is done for consistency with Forms, and it would be a major backwards incompatibility to change this behavior.