Opened 13 years ago

Closed 13 years ago

Last modified 13 years ago

#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 Russell Keith-Magee, 13 years ago

Resolution: invalid
Status: newclosed

Like the comment says, this is done for consistency with Forms, and it would be a major backwards incompatibility to change this behavior.

comment:2 by Tim Molendijk, 13 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

in reply to:  2 comment:3 by Łukasz Rekucki, 13 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.

Note: See TracTickets for help on using tickets.
Back to Top