7

Please help me understand if the following choices I have made are idiomatic/good and if not, how to improve.

1) Model Validation over Form Validation
I prefer to use the new model validation over form validation whenever possible as it seems a more DRY and fundamental way to create rules for the data. Two examples for a simple calendar-entry model:

  • "start must be before end"
  • "period must be less than (end-start)"

Is it idiomatic/good to put those at the model level so they don't have to be put into forms? If ModelForm is the best answer, then what about the case of using multiple models in the same form?
(edit: I didn't realize that multiple ModelForms can actually be used together)

2) Transferring Model Validation to a Form (not ModelForm)
(edit: It turns out reinventing the plumbing between model validation and form validation is not necessary in my situation and the solutions below show why)
Let's assume for a moment that any model validation errors I have can be directly transferred and displayed directly to the user (i.e. ignore translating model validation errors to user-friendly form validation errors).

This is one working way I came up with to do it (all in one place, no helper functions):

view_with_a_form:
...
if form.is_valid():
    instance = ...
    ...
    #save() is overridden to do full_clean with optional exclusions
    try: instance.save()
    except ValidationError e:
        nfe= e.message_dict[NON_FIELD_ERRORS]
        #this is one big question mark:
        instance._errors['__all__'] = ErrorList(nfe)
#this is the other big question mark. seems unnatural
if form.is_valid()
    return response...
... #other code in standard format
#send the user the form as 1)new 2)form errors 3)model errors
return form

As commented in the code:
a) Is this an idiomatic/good way to transfer model errors to a form?

b) Is this an idiomatic/good way to test for new "form" errors?

Note: This example uses non-field errors, but I think it could equally apply to field errors.

KobeJohn
  • 7,390
  • 6
  • 41
  • 62
  • I'm changing the selected answer since the updates from Stefano actually convinced me to adapt another method ([one of the combined ModelForms methods here](http://stackoverflow.com/questions/569468/django-multiple-models-in-one-template-using-forms)) without needing to reinvent the plumbing between model validation and form validation. (I wasn't aware that they could be combined). Chris' answer is great also to the best of my understanding. – KobeJohn Oct 13 '11 at 10:42

2 Answers2

5

[Edited - hope this answers your comments]

I'll be brief and direct but do not want be impolite :)

1) Model Validation over Form Validation

I guess that the rule of thumb could be that as long as the rule is really associated with the model, yes, it's better to do validation at model level.

For multiple-model forms, please checkout this other SO question: Django: multiple models in one template using forms, not forgetting the attached link which is slightly old but still relevant on the dryest way to achieve it. It's too long to re-discuss it all here!

2)

  • a) No! You should derive your form from a ModelForm that will perform model validation for you -> you do not need to call model validation yourself
  • b) No! If you want to validate a model you should not try and save it; you should use full_clean -> so if your ModelForm is valid, then you know the model is valid too and you can save.

And yes, these answers still apply even with multi-model forms.

So what you should do is:

  • Still use ModelForm
  • do not worry about validating a model, because a ModelForm will do that for you.

In general if you find yourself doing some plumbing with django it's because there's another, simpler way to do it!

The 2nd pointer should get you started and actually simplify your code a lot...

Community
  • 1
  • 1
Stefano
  • 18,083
  • 13
  • 64
  • 79
  • As for part 2), let me respond with a few points to understand better. a) In this case, yes I could use a ModelForm more cleanly. However, I should have stated that I have forms that incorporate several models, so a ModelForm wouldn't be appropriate would it? In this case, is the way I did it ok? b) Fair enough. I prefer it in save since so far, that's the only combination I've found. full_clean followed by save if no problems. So I just combined them rather than needing to remember it in every field. More DRY I thought. – KobeJohn Oct 12 '11 at 13:30
  • @yakiimo hope I answered your additional questions – Stefano Oct 12 '11 at 17:48
5

1) Yes, it's perfectly valid to do validation on the model. That's why the Django folks added it. Forms aren't always used in the process of saving models, so if the validation is only done through forms, you'll have problems. Of course, people worked around this limitation in the past by overriding the save method and including validation that way. However, the new model validation is more semantic and gives you a hook into the validation process when actually using a form.

2) The docs pretty clearly say that model validation (full_clean) is run when ModelForm.is_valid is called. However, if you don't use a ModelForm or want to do additional processing, you need to call full_clean manually. You're doing that, but putting it in an overridden save is the wrong approach. Remember: "Explicit is better than implicit." Besides, save is called in many other places and ways, and in the case of a ModelForm, you'll actually end up running full_clean twice that way.

That said, since the docs say that ModelForm does full_clean automatically, I figured it would make sense to see how it handles errors. In the _post_clean method, starting on line 323 of django.forms.models:

# Clean the model instance's fields.
try:
    self.instance.clean_fields(exclude=exclude)
except ValidationError, e:
    self._update_errors(e.message_dict)

# Call the model instance's clean method.
try:
    self.instance.clean()
except ValidationError, e:
    self._update_errors({NON_FIELD_ERRORS: e.messages})

In turn, the code for _update_errors starts on line 248 of the same module:

def _update_errors(self, message_dict):
    for k, v in message_dict.items():
        if k != NON_FIELD_ERRORS:
            self._errors.setdefault(k, self.error_class()).extend(v)
            # Remove the data from the cleaned_data dict since it was invalid
            if k in self.cleaned_data:
                del self.cleaned_data[k]
    if NON_FIELD_ERRORS in message_dict:
        messages = message_dict[NON_FIELD_ERRORS]
        self._errors.setdefault(NON_FIELD_ERRORS, self.error_class()).extend(messages)

You'll have to play with the code a bit, but that should give you a good starting point for combining the form and model validation errors.

Chris Pratt
  • 232,153
  • 36
  • 385
  • 444
  • Good call on looking at the source code for ModelForm. That had crossed my mind and subsequently passed out of it. I'll look into it now. Thanks for all parts of your reply. – KobeJohn Oct 12 '11 at 16:16
  • After checking out the dev code and looking further, it looks like more than enough rope to hang myself with at my skill level. Hopefully the combined forms will work out. Thanks again for the reply and leads. – KobeJohn Oct 13 '11 at 10:52