Code

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#15323 closed (invalid)

django.db.models.Model.clean() should be independent from .clean_fields()

Reported by: timmolendijk Owned by: nobody
Component: Database layer (models, ORM) Version: master
Severity: Keywords: model validation clean
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

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

Attachments (0)

Change History (3)

comment:1 Changed 3 years ago by russellm

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to invalid
  • Status changed from new to closed

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 follow-up: Changed 3 years ago by 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

comment:3 in reply to: ↑ 2 Changed 3 years ago by lrekucki

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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.