Documenting reporting errors on individual fields in Model.clean (#16986)

[Alasdair Nicol]

Hi,

Ticket #16986 (Model.clean cannot report errors on individual fields)
was fixed in Django 1.7, but the new feature is not documented. The
Model.clean() docs still state:

> Any ValidationError exceptions raised by Model.clean() will be stored
> in a special key error dictionary key, NON_FIELD_ERRORS, that is used
> for errors that are tied to the entire model instead of to a specific
> field

The syntax to report an error on an individual field is to pass a dict
to the ValidationError constructor.

> def clean(self):
> if self.name1 == 'FORBIDDEN_VALUE':
> raise ValidationError({'name1':
[ValidationError('Model.clean() error messages.')]})

However, this is a private API. There seemed to be some debate in ticket
#20867 about whether or not to document it.

I'd be happy work on a patch to update the documentation, but I wanted
to check that it is ok to document this API.

cheers,
Alasdair

--
Alasdair Nicol
Developer, MEMSET

mail: alas...@memset.com
web: http://www.memset.com/

Memset Ltd., registration number 4504980.
Building 87, Dunsfold Park, Stovolds Hill, Cranleigh, Surrey, GU6 8TB, UK

[Carl Meyer]

Hi Alasdair,

On 11/17/2014 09:05 AM, Alasdair Nicol wrote:
> Ticket #16986 (Model.clean cannot report errors on individual fields)
> was fixed in Django 1.7, but the new feature is not documented. The
> Model.clean() docs still state:
>
>> Any ValidationError exceptions raised by Model.clean() will be stored
>> in a special key error dictionary key, NON_FIELD_ERRORS, that is used
>> for errors that are tied to the entire model instead of to a specific
>> field
>
> The syntax to report an error on an individual field is to pass a dict
> to the ValidationError constructor.
>
>> def clean(self):
>> if self.name1 == 'FORBIDDEN_VALUE':
>> raise ValidationError({'name1': [ValidationError('Model.clean()
> error messages.')]})
>
> However, this is a private API. There seemed to be some debate in ticket
> #20867 about whether or not to document it.
>
> I'd be happy work on a patch to update the documentation, but I wanted
> to check that it is ok to document this API.

I think this should be documented. It's a good use case, and a
reasonable API for it. Several people asked in both #16986 and #20867
about documenting it, and it looks to me like the general tone was in
favor. Nobody spoke against it, but nobody ever gave a firm answer so
nothing happened.

Carl

[Tim Graham]

The reasoning for why it wasn't documented is provided here: https://github.com/django/django/pull/1443#issuecomment-28045150

[Carl Meyer]

On 11/17/2014 09:48 AM, Tim Graham wrote:
> The reasoning for why it wasn't documented is provided here:
> https://github.com/django/django/pull/1443#issuecomment-28045150

That comment is actually in favor of documenting it for Model.clean(),
just suggested it be done in a separate PR.

(Personally I think it would be better to document the same technique
for both Model.clean() and Form.clean(), because I don't think
Form.add_error() is significantly easier, and it breaks the API
consistency of "always flag a validation problem by raising
ValidationError", but I guess that's a larger discussion.)

Carl

[Alasdair Nicol]

Hi,

Thank you Carl and Tim for your replies. I've created a ticket #23865
and written a patch [1]for the Model.clean() docs [1].

I've been updating a codebase to Django 1.7 this week. Personally, I
find the add_error API to be more concise when you don't want to raise
an exception immediately.

class MyForm(forms.Form):
def clean():
if check1():
self.add_error('field1', 'error message')
if check2():
self.add_error('field2', 'error message')

In my models' clean methods, I have to keep track of the errors as I go
along.

class MyModel(models.Model):
def clean():
errors = {}
if check1():
errors['field1'] = 'error message'
if check2():
errors['field2'] = 'error message'
if errors:
raise ValidationError(errors)

Having said that, it's not that much more difficult, only 3 extra lines
of code in this case. API consistency is a good argument for documenting
the ValidationError(dict) approach in both places.

Cheers,

[Carl Meyer]

Yes, the add_error() API is slightly less verbose if you have multiple
different errors you may want to flag. I don't think that difference is
worth having to remember two different ways to do it, but I don't really
feel strongly enough about it to push for what would essentially amount
to deprecating `Form.add_error()`.

I am satisfied as long as `raise ValidationError(dict)` continues to
work in both places, so I can use it in my code :-)

Carl

[Loic Bistuer]

Neither will go away, `ValidationError(dict)` is used internally to carry errors from the model layer to the form layer, and `add_error()` is used internally to interpret the `ValidationError` raised throughout the system; basically any `raise ValidationError` is followed by a call to `add_error()`.

One benefit of `add_error()` is that it's available from outside the form validation methods, it's much less fiddly to add an error from a view if you are using `add_error()`. It also doubles as a low-level API that can be used to implement custom validation logic, for instance `ModelForm` uses it to plug in model validation.

Fore more background on `add_error()` as a public API: https://code.djangoproject.com/ticket/20867#comment:4

--
Loic

[Anssi Kääriäinen]

One possible way forward is to add "errors" argument to the clean()
method. Then it would be possible to use the same API everywhere by
doing errors.add_error('my_field', 'My Error message').

I don't know how strongly we feel about breakages caused by new
arguments to methods. We broke model.save() this way some time ago IIRC.

- Anssi

[Carl Meyer]

I don't think that spreading add_error to more places really helps with
the "more than one way to do it" issue - raising ValidationError isn't
going anywhere as documented public API. It's still the simplest thing
to do from a clean_field method, or if you want the error in
non_field_errors.

I realize that add_error() is a useful internal API, just not clear that
it makes sense to have two public APIs for adding errors. "Easier to add
errors from a view" doesn't carry weight for me: poking errors into a
form from outside the form is smelly (violates the encapsulation of the
form class), so I think it's good if it also looks smelly, rather than
being easy.

But as I said before, the "add_error() as public API" ship has already
sailed, and I don't believe that it's worth trying to reverse its course
now. I am in favor of Alasdair's current PR simply documenting
ValidationError(dict) for model validation.

Carl