Cleaning woes

[tykling]

Background

Figure out how we want to clean/validate data (complex business rule validation, like "is this EventLocation available at this time?" rather than "is this value an integer?").

This rant is not specific to the BornHack website project, the points are about Django in general!

Requirements:

• Must play nice with CBVs and form.save(commit=False) saves.
• Must always maintain data integrity according to our business logic, regardless of how we save() - except if we choose to skip validation for a given save().
• Must give nice error messages in the form when a problem is found

The Django Way is Insufficient

• Model clean() methods are called by form.save(commit=False) so they are not terribly useful, because they often prevent saving because stuff like self.camp and self.user is not set yet, if the validation requires these values.
• ModelForms clean() methods handle errors well, but they are not called when saving in the admin, console, management commands. So they do not maintain data integrity if the form is not used.
• Model save() methods can be used, but do not allow us to show a pretty error message in the form. This is why we always get a 500 when trying to edit a readonly camp.
• Model Meta ExclusionConstraints can not be skipped (because they are in the database not in Django), and they can not show a pretty error message in the form.

What do we do?

The best option I've come up with until now is to define one or more cleaning methods on the model (called something other than clean()) which are called from form_valid() before the final save, and if a ValidationError is raised we add the exception to the form object with form.add_error(None, E) and return self.form_invalid(form).

This takes care of the pretty error messages (as long as one remembers to call the clean method(s) from form_valid()).

The same clean method(s) are called from the models save() method, if "commit" not in kwargs, or kwargs["commit"] is True. This takes care of cleaning whenever we save() (be it from admin, console or whatever), except when it is a form.save(commit=False) thing.

The models save() method can optionally be equipped with one or more new kwargs (defaulting to True) like clean_availability or clean_speakers and so on, which can be set to False when calling save() to skip the validation if needed. This means we can easily make a "skip validation" checkbox in a form if we need to.

With this approach the only way data can get into the database without validation is through the bulk_create() and other methods that skip .save() entirely.

What an absolute faff. Why is this so difficult, what am I missing? Is there a better way?

Future

I'd love to standardise how we do this across the project. But first we have to find the right way to do it!

[benjaoming]

We can't require these 2 at the same time, I think?

• Must always maintain data integrity according to our business logic

...

• Can be skipped when saving should the need arise

[benjaoming]

ModelForms clean() methods handle errors well, but they are not called when saving in the admin, console, management commands. So they do not maintain data integrity if the form is not used.

I think this one can be addressed: Normally, a ModelAdmin generates its own ModelForm from the various settings in ModelAdmin -- like which fields to expose etc. But it also supports ModelAdmin.form (see docs), so we can create better and more reusable classes or mixins for ModelForm that can be used in both frontend and django-admin.

[benjaoming]

What do we do?

It's an interesting proposal to have forms call a form_valid and get a list of additional fields and error messages.

Can you outline the call structure? Or maybe implement a minimal example?

It's hard to quickly grasp how form.clean(), model.clean() and individual form and model field clean methods already contributes quite a big structure -- yes, even model.save(). It would be a shame to get ambiguous patterns or have to re-implement some of these.

[tykling]

Thinking about this, model.clean() is actually usually only called by the form framework, we just call it in the bornhack project "basemodel" .save() (which all other models inherit from), to make sure stuff is actually cleaned (in a previous attempt to find heads or tails in this).

There might be an easy fix actually, I can just pass the commit kwarg to the model clean() and I'll be golden. Maybe, I'll check later.

But that still only fixes it for the bornhack-website project. The rest of the Django world still has this weird situation where data apparently only needs cleaning when it comes from a form.

[ramarrom]

Independent of the checking in Django, I like to put it in the database whenever I can. Then maybe the web request fails, but at least the database is fine.