Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#20867 closed New feature (fixed)

Allow Form.clean() to target specific fields when raising ValidationError.

Reported by: loic84 Owned by: loic84
Component: Forms Version: master
Severity: Normal Keywords:
Cc: hv@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no


The goal would be to unlock the following API:

def clean(self):
    errors = {}
    if condition1:
        errors['field1'] = ValidationError('message1', code='code')
    if condition2:
        errors['field2'] = ValidationError('message2', code='code')
    raise ValidationError(errors)

Most of the groundwork has been done for #20199.

We would need to document passing a dict to ValidationError, just like we already document passing a list in ref/forms/validation/#raising-validationerror.

This would probably deprecate or reduce the need for the pattern discussed in ref/forms/validation/#form-subclasses-and-modifying-field-errors.

Refs #20199 #16986.

Change History (12)

comment:1 Changed 7 years ago by loic84

Has patch: set
Needs documentation: set
Owner: changed from nobody to loic84
Patch needs improvement: set
Status: newassigned

comment:2 Changed 7 years ago by Tim Graham

Triage Stage: UnreviewedAccepted

comment:3 Changed 7 years ago by loic84

Needs documentation: unset
Patch needs improvement: unset

comment:4 Changed 7 years ago by loic84

Quite simultaneously yet in different contexts @mjtamlyn and @akaariai had the idea of having a Form method to manipulate the _error dict.

@akaariai on IRC:

(07:56:59 AM) akaariai: I am reading and wondering why this must be so complicated? Why not Form.add_error(field, message)?
(07:59:26 AM) jtiai: akaariai: Maybe because no one bothered to figure out API for that?
(07:59:53 AM) timograham: akaariai: WIP by loic84 is
(08:04:04 AM) akaariai: timograham: hmmh, seems to me that having self.add_error instead of documenting self._errors would be an improvement even if ValidationError(errors_dict) went in.
(08:05:19 AM) akaariai: there are some cases where you want to add errors post-clean, and just getting rid of that self._errors docs seems nice.
(08:06:08 AM) akaariai: compare that to add_error(field, error_message): Adds an error to specified field. If field is None, then adds a non_field_error.

@mjtamlyn talking about a private utility method that I used in the PR above:

[...] It could even be made public - so this is a shortcut for the old way of doing things, with the clever ValidationError exceptions being a further improvement.

As a result I've cleaned up my utility method and replaced it with Form.add_errors(self, field, errors).

Now the question is what do we do in term of public API, the current patch documents raising ValidationError with a dictionary argument. We could also document add_errors(), or we could only document add_errors().

One good thing in having a public add_errors() is that it would be accessible from outside the form. Many time I wished I could add some errors to a form instance from a view without having to fiddle with _errors.

comment:5 Changed 7 years ago by Marc Tamlyn

This got me wondering a bit, is there any need for ValidationError to support dictionaries if we have add_errors? The original aim was to make clean() methods nicer when affecting multiple fields.

comment:6 Changed 7 years ago by loic84

We need good support for dict because that's how errors travel from the Model layer to the Form layer. Also ValidationError is the vehicle to carry metadata like error code and error params which are lost as soon as they reach ErrorDict. That said, we don't need to advertise it.

Last edited 7 years ago by loic84 (previous) (diff)

comment:7 Changed 7 years ago by Marc Tamlyn

Yup, of course. I think it's worth documenting both personally.

comment:8 Changed 7 years ago by Marc Tamlyn

Any resolution to this will not also include the add_errors() functionality, which fixes #5335. I've closed that ticket as its topic has been subsumed into this one.

Version 0, edited 7 years ago by Marc Tamlyn (next)

comment:9 Changed 7 years ago by Thomas Güttler

Cc: hv@… added

comment:10 Changed 7 years ago by loic84

I've updated the PR.

I went with the add_error(field, error) API (note the singular) which seems to be the most popular from the feedback I've received on IRC and on the ML thread

comment:11 Changed 7 years ago by Loic Bistuer <loic.bistuer@…>

Resolution: fixed
Status: assignedclosed

In f563c339ca2eed81706ab17726c79a6f00d7c553:

Fixed #20867 -- Added the Form.add_error() method.

Refs #20199 #16986.

Thanks @akaariai, @bmispelon, @mjtamlyn, @timgraham for the reviews.

comment:12 Changed 7 years ago by Loic Bistuer <loic.bistuer@…>

In 2e3c7d882015375c130c21884d83cb9fb7759d94:

Trigger AttributeError in ValidationError.message_dict when error_dict is missing.

The goal of this change is twofold; firstly, matching the behavior of Django 1.6
and secondly, an AttributeError is more informative than an obscure ValueError
about mismatching sequence lengths.

Refs #20867.

Note: See TracTickets for help on using tickets.
Back to Top