Opened 7 years ago

Closed 7 years ago

Last modified 6 years ago

#6652 closed (fixed)

Allow 'clean()' to specify the wrong field

Reported by: Daniele Varrazzo <piro@…> Owned by: nobody
Component: Forms Version: master
Severity: Keywords: feature
Cc: Triage Stage: Design decision needed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

Currently the Form.clean() method has no way to specify which field is invalid, but it's the only place where validation involving many fields can be accomplished. Taking your example in the documentation:

This is where you might put in things to check that if field A is supplied, field B must contain a valid email address and the like.

If the field B doesn't contain a valid email address, the error message should appear next to the B widget, not at the top of the form.

An easy fix is to add a 'field' optional attribute to the ValidationError and use it if provided. I already used this method in other programs of mine (my exception class was called ValidationError too!)

Attachments (3)

field-on-validation-error.patch (1.1 KB) - added by Daniele Varrazzo <piro@…> 7 years ago.
allow_clean_to_specify_the_wrong_field-with-docs.diff (4.7 KB) - added by mrts 7 years ago.
allow_clean_to_specify_the_wrong_field-updated-tests-docs.diff (8.3 KB) - added by mrts 7 years ago.
Appends errors from clean() to original errors.

Download all attachments as: .zip

Change History (24)

Changed 7 years ago by Daniele Varrazzo <piro@…>

comment:1 Changed 7 years ago by Daniele Varrazzo <piro@…>

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

An example of use:

    def clean(self):
        # Check the gateway is compatible with the address/mask
        gateway = self.cleaned_data['gateway']
        address = self.cleaned_data['address']
        netmask = self.cleaned_data['netmask']
        if not netconfig.addresses_match(address, gateway, netmask):
            raise forms.ValidationError(
                "Gateway not valid for the specified address",
                field='gateway')

comment:2 follow-up: Changed 7 years ago by russellm

  • Resolution set to wontfix
  • Status changed from new to closed

You're missing the obvious solution - use a clean_field method (eg clean_email()). A field specific clean method can reference other entries from cleaned_data[] if necessary, and any error message will be associated with the field, rather than the form.

comment:3 Changed 7 years ago by mtredinnick

  • Summary changed from [feature request] allow 'clean()' to specify the wrong field to Allow 'clean()' to specify the wrong field

Russell: that does impose an ordering constraint, though -- dependencies done this way can only depend on fields before themselves.

I'd probably be +0 on the patch since it has no backwards compatibility impact. Alternatively, we could document how to add to Form._errors. Leaving closed for now, though.

comment:4 in reply to: ↑ 2 Changed 7 years ago by Daniele Varrazzo <piro@…>

Replying to russellm:

You're missing the obvious solution - use a clean_field method (eg clean_email()).

Not so obvious: when clean_email() is called you may not have yet the cleaned results of all the fields required for the composed validation.

comment:5 follow-up: Changed 7 years ago by russellm

  • Keywords feature added

Malcolm: Valid point about the ordering requirement, although I'm not convinced that it is a problem in practice. The error message needs to be tied to a specific field, and you can always set up the field order so that the field with the error is last.

This enforces an order for presentation that may not be natural, but only matters if you are rendering using form.as_*; if presentation order doesn't match logical order, you can manually render each field.

My rejection of this idea was based on the fact that it introduces a second way to raise field errors. However, I'm only a -0. Regardless, better documentation of Form._errors is probably a good idea.

comment:6 in reply to: ↑ 5 Changed 7 years ago by Daniele Varrazzo <piro@…>

Replying to russellm:

My rejection of this idea was based on the fact that it introduces a second way to raise field errors. However, I'm only a -0. Regardless, better documentation of Form._errors is probably a good idea.

Are you sure exposing Form._errors is a good idea? Currently it is an implementation detail (at least for people developing form logic): exposing it in the API is an implementation leak that would tie the current implementation to everybody's form logic.

Adding an optional field to the exception is a (mostly harmless) extension of the current interface.

comment:7 Changed 7 years ago by anonymous

The other point is that when you are composing forms from multiple base forms, you don't always have direct control over ordering unless you redeclare fields (which defeats the purpose of multiple base forms in part).

comment:8 Changed 7 years ago by anonymous

Considering the strong points given, shouldn't this be reopened? I for one would like to use it like in the example above, rather than having to depend on field ordering.

comment:9 Changed 7 years ago by anonymous

  • Resolution wontfix deleted
  • Status changed from closed to reopened

comment:10 Changed 7 years ago by jacob

  • Triage Stage changed from Unreviewed to Ready for checkin

comment:11 Changed 7 years ago by jacob

  • Needs documentation set
  • Needs tests set
  • Triage Stage changed from Ready for checkin to Accepted

comment:12 Changed 7 years ago by mrts

I'm also using this in my projects, so I modified the patch to add tests and documentation (within the tests).

I think it is more clear to use field_name instead of field in the exception (using field is against the DON'T MAKE ME THINK principle: what's a field? instance? name?).

Find the patch attached.

comment:13 Changed 7 years ago by anonymous

  • Needs documentation unset
  • Needs tests unset

comment:14 Changed 7 years ago by mrts

  • Triage Stage changed from Accepted to Ready for checkin

Improved patch against [7321] that appends errors from clean() to base field errors.

Changed 7 years ago by mrts

Appends errors from clean() to original errors.

comment:15 Changed 7 years ago by mtredinnick

  • Triage Stage changed from Ready for checkin to Design decision needed

Given the currently active discussion on django-dev regarding changes to model and form validation (including some renaming), let's sit on this for a bit.

comment:16 Changed 7 years ago by mrts

  • milestone set to post-1.0

Not in scope for 1.0.

comment:17 Changed 7 years ago by rfugger

I just had to fiddle with Form._errors to solve this problem and I think being able to direct an error to a specific field using ValidationError is by far more elegant and useful. Please adopt this solution. Thanks.

comment:18 Changed 7 years ago by mtredinnick

I don't like the approach proposed here now that I've looked at it a bit more. ValidationError is an exception that should be raised and is then caught by the outer loop (in normal single-field processing) and stored in the ErrorList. A validation error is the exception instance, not tied to a field. The conceptual problem here is that Form.clean() is for validating multiple fields at once, so you don't want to raise an exception on the first problem: you still want to handle all errors and it's not part of some outer loop that gets called again and again.

Documenting _errors, which is designed to hold the errors and can be happily used by subclasses is a better approach here. I'll do that today and then close this.

comment:19 Changed 7 years ago by mtredinnick

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

(In [9177]) Added a lot more explanation about form field validation, including expanded
examples. Fixed #5843, #6652, #7428.

comment:20 Changed 7 years ago by mtredinnick

(In [9180]) [1.0.X] Added a lot more explanation about form field validation, including
expanded examples. Fixed #5843, #6652, #7428.

Backport of r9177 from trunk.

comment:21 Changed 6 years ago by anonymous

  • milestone post-1.0 deleted

Milestone post-1.0 deleted

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