Downsides of calling Model.clean() from Model.save()?

[TheBeardedTemplar]

Hey all,

I have a model with some fairly strict rules on what is or isn't allowed and I'd like to perform some validation. The model is created via various forms, other functions, and via api calls and so validating the data before the model is created would be quite complicated. Reading on Stack Overflow, people seem to caution against overriding the Model.save() function to call a clean function which checks for these issues. However all the posts I've seen on it were from Django 1.2 or 1.3 and I'm not certain if they're still relevant. So Is there a reason not to do it this way? If so, what would a good solution be.

Example Code:

class MyModel:
   
    def save(self, *args, **kwargs):
        """
        Override the save function to provide some validation on the model.
        """
        self.clean()
        super(MyModel, self).save(*args, **kwargs)


    def clean(self):
        if badThing == True:

            raise ValidationError("This data is bad and you should feel bad")
   

[James Schneider]

On Sep 11, 2015 3:25 PM, "TheBeardedTemplar" <thebeard...@gmail.com> wrote:

Hey all,

I have a model with some fairly strict rules on what is or isn't allowed and I'd like to perform some validation. The model is created via various forms, other functions, and via api calls and so validating the data before the model is created would be quite complicated. Reading on Stack Overflow, people seem to caution against overriding the Model.save() function to call a clean function which checks for these issues. However all the posts I've seen on it were from Django 1.2 or 1.3 and I'm not certain if they're still relevant. So Is there a reason not to do it this way? If so, what would a good solution be.

Personal opinion and experience; the reason for keeping validation separate from storage is in the handling of validation logic (especially with regards to failed validation behavior).

If your model fails validation, what is the save() method supposed to do? You would have to guarantee that every place that model.save() is called is also capable of handing a validation error. Often this isn't the case, as validation is often assumed to be completed successfully by the time model.save() is called (usually by a form, external to the model). ModelForm validation will also call the .clean() methods for the model instance. If you were to place a reference to clean in save(), the validation code would run twice. Might not be a problem, but it is at the very least inefficient and non-intuitive, and likely to introduce bugs if your cleaning methods are not idempotent. 

Being explicit in calling your clean() methods follows more closely with the Zen of Python (PEP 20). Yes, it is an extra line. Yes, you'll need to remember to add it wherever you want validation to occur before saving an object. Yes, this also means you'll need to write code to handle validation failures. No, it doesn't conflict with the DRY principle since that line of code is specific only to the operation you are performing (others may feel differently, but explicit > DRY, usually).

I think you'll find as you dive in deeper and start using the objects in this way that it makes sense not to combine both operations under one save() umbrella, especially since you mention multiple input methods for creating objects (API, web form, external functions). Each of those will likely have (slightly) different validation behaviors. If they don't, then write a function called validate_and_save(obj) and place your obj.clean() and obj.save() code in there. Sounds like the same thing, but much easier to track down later, and it leaves you with the flexibility of calling obj.save() directly later without running through validation (perhaps creating objects from known-good data, or ModelForms are in use, etc.).

HTH,

-James