Code

Opened 11 months ago

Closed 8 months ago

Last modified 7 months 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

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.

Attachments (0)

Change History (12)

comment:1 Changed 11 months ago by loic84

  • Has patch set
  • Needs documentation set
  • Needs tests unset
  • Owner changed from nobody to loic84
  • Patch needs improvement set
  • Status changed from new to assigned

comment:2 Changed 11 months ago by timo

  • Triage Stage changed from Unreviewed to Accepted

comment:3 Changed 11 months ago by loic84

  • Needs documentation unset
  • Patch needs improvement unset

comment:4 Changed 11 months 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 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 Changed 11 months ago by mjtamlyn

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

comment:7 Changed 11 months ago by mjtamlyn

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

comment:8 Changed 11 months ago by mjtamlyn

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

[edited not->now]

Last edited 11 months ago by mjtamlyn (previous) (diff)

comment:9 Changed 9 months ago by guettli

  • Cc hv@… added

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

comment:11 Changed 8 months ago by Loic Bistuer <loic.bistuer@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

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 months 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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.