Opened 11 years ago

Closed 11 years ago

Last modified 11 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: dev
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

Description

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 by loic84, 11 years ago

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

comment:2 by Tim Graham, 11 years ago

Triage Stage: UnreviewedAccepted

comment:3 by loic84, 11 years ago

Needs documentation: unset
Patch needs improvement: unset

comment:4 by loic84, 11 years ago

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 https://docs.djangoproject.com/en/dev/ref/forms/validation/#form-subclasses-and-modifying-field-errors 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 https://github.com/django/django/pull/1443
(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 by Marc Tamlyn, 11 years ago

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 by loic84, 11 years ago

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 11 years ago by loic84 (previous) (diff)

comment:7 by Marc Tamlyn, 11 years ago

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

comment:8 by Marc Tamlyn, 11 years ago

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 11 years ago by Marc Tamlyn (next)

comment:9 by Thomas Güttler, 11 years ago

Cc: hv@… added

comment:10 by loic84, 11 years ago

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 https://groups.google.com/d/topic/django-developers/rTbfg3JtLkA/discussion.

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

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 by Loic Bistuer <loic.bistuer@…>, 11 years ago

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