Opened 4 years ago

Closed 2 years ago

Last modified 21 months ago

#16986 closed New feature (fixed)

Model.clean cannot report errors on individual fields

Reported by: davidfstr Owned by: nobody
Component: Forms Version: master
Severity: Normal Keywords:
Cc: davidfstr, danny.adair@…, kmike84@…, gp, julie@…, james@…, marky1991, loic@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Any ValidationError raised by Model.clean() is always attributed to "__all__" (NON_FIELD_ERRORS), even if the ValidationError was created with a dictionary that associated errors with particular fields.

For example, in the following code...

class Spot(models.Model):
    def clean(self):
        errors = defaultdict(list)
        if not self.name and not self.address_line_1:
            message = 'Spots must have either a name, address, or both specified.'
            errors['name'].append(message)
            errors['address_line_1'].append(message)
        if len(errors):
            raise ValidationError(errors)

...even though the error dictionary was specified explicitly, the dictionary seen by Django is:

    errors['__all__'].append(message) # expected: name
    errors['__all__'].append(message) # expected: address_line_1

I have traced the issue to BaseModelForm._post_clean: it ignores the error dictionary completely, using the error list instead (which lacks the field name associations).

I have attached a patch. It passes all unit tests.

Attachments (5)

model-clean-errordict.diff (619 bytes) - added by davidfstr 4 years ago.
model-clean-errordict-2.diff (2.0 KB) - added by davidfstr 4 years ago.
model-clean-errordict-3.diff (5.3 KB) - added by davidfstr 4 years ago.
model-clean-errordict-4.diff (7.0 KB) - added by davidfstr 4 years ago.
model_clean_mixin.py (2.3 KB) - added by davidfstr 4 years ago.

Download all attachments as: .zip

Change History (31)

Changed 4 years ago by davidfstr

comment:1 Changed 4 years ago by carljm

  • Needs documentation set
  • Needs tests set
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Bug to New feature

Thanks for the report and patch!

I'd call this a new feature rather than a bug, since it's never been documented anywhere that you can raise a dict-style ValidationError from clean to get errors associated with particular fields. That said, it's a useful feature and one that I've wanted before, and the implementation is trivial and non-invasive.

Rather than spreading the hasattr(..., "message_dict") disease outside ValidationError, I think the patch should introduce a new has_dict property on ValidationError that keeps that check internal to the implementation (and may as well clean up the internal hasattr uses to use the new property too).

Also, a note should be added to the docs that this is possible, and showing how its done. And we need a new test demonstrating that it works.

Last edited 4 years ago by carljm (previous) (diff)

comment:2 Changed 4 years ago by davidfstr

  • Cc davidfstr added

(Hopefully the CC change will subscribe me to future updates)

Agreed that it would be good to isolate hasattr(..., "message_dict") logic.

I have a different approach though: make ValidationError.message_dict a property that self-initializes to {'__all__': self.messages}. I need to check whether this affects backward-compatibility though. Then I'll look at updating the patch.

comment:3 follow-up: Changed 4 years ago by davidfstr

Regarding backward-compatibility, there is unfortunately very little documentation for ValidationError, so I must assume any advanced users are depending on the exact implementation.

So I've reverted to your implementation approach (which has minimal impact) and updated the patch. I recognize it still needs a new test case and documentation changes.

Changed 4 years ago by davidfstr

comment:4 in reply to: ↑ 3 ; follow-up: Changed 4 years ago by carljm

Replying to davidfstr:

Regarding backward-compatibility, there is unfortunately very little documentation for ValidationError, so I must assume any advanced users are depending on the exact implementation.

So I've reverted to your implementation approach (which has minimal impact) and updated the patch. I recognize it still needs a new test case and documentation changes.

Actually, the fact that the internals of ValidationError are not documented means that they are considered an implementation detail, and can be changed without regard to backward-compatibility; only the documented interface (which in this case I think is just instantiation with a message string) need remain compatible. Anyone using undocumented internal details can update their code if needed.

That said, I'm not sure initializing message_dict the way you suggest is correct. ValidationError without a dict can be used from inside a clean_field method to raise errors specific to that field, in which case having a message_dict property auto-initialized using __all__ seems semantically wrong. Essentially ValidationError itself can in some cases be agnostic as to which field its errors belong to, and that is determined by context.

comment:5 Changed 4 years ago by danny.adair@…

  • Cc danny.adair@… added

comment:6 in reply to: ↑ 4 Changed 4 years ago by davidfstr

  • Needs tests unset

Replying to carljm:

That said, I'm not sure initializing message_dict the way you suggest is correct. ValidationError without a dict can be used from inside a clean_field method to raise errors specific to that field, in which case having a message_dict property auto-initialized using __all__ seems semantically wrong. Essentially ValidationError itself can in some cases be agnostic as to which field its errors belong to, and that is determined by context.

Interesting. I didn't realize that ValidationError could be used in that way. I guess I haven't written any custom fields with their own validation logic.

Anyway, I've update the patch with unit tests, now that I've figured out how Django's unit tests work.

P.S. Apparently it wasn't sufficient to add myself to the CC list to receive emails. I've now updated my Trac profile with my email address. Hopefully I should now receive email updates. And be able to respond in a more timely fashion.

Changed 4 years ago by davidfstr

comment:7 Changed 4 years ago by davidfstr

  • Needs documentation unset

Added documentation. All patch components complete. Please review.

Changed 4 years ago by davidfstr

comment:8 Changed 4 years ago by davidfstr

  • Owner changed from nobody to davidfstr

comment:9 Changed 4 years ago by davidfstr

  • Patch needs improvement unset

Clearing the "Patch needs improvement" flag, based on guidance from https://docs.djangoproject.com/en/dev/internals/contributing/triaging-tickets/.

comment:10 follow-up: Changed 4 years ago by kmtracey

Adding this makes sense to me...however I'm a little curious if there was a good reason for this not being done by r12402.

comment:11 in reply to: ↑ 10 Changed 4 years ago by anonymous

This is what I am able to piece together:

  1. Pre r12402, it was possible to raise a ValidationError with a dictionary but not a list of messages.
  2. Post r12402, it was possible to raise a ValidationError with a list of messages but not a dictionary. (That is, it was a breaking change.)
  3. This patch allows a ValidationError to be raised with either a list of messages or a dictionary.

Based on the wording of r12402, it was originally that case (2) definitely work but that case (1) had not been considered seriously. This patch allows both cases (1) and (2) to work.

comment:12 Changed 4 years ago by davidfstr

(anonymous poster above is me)

comment:13 follow-up: Changed 4 years ago by davidfstr

Is there any futher action on my part that is preventing this ticket from proceeding?

comment:14 in reply to: ↑ 13 ; follow-up: Changed 4 years ago by carljm

  • Patch needs improvement set

Replying to davidfstr:

Is there any futher action on my part that is preventing this ticket from proceeding?

Nothing preventing, no. You always have the option of finding some other community member to review the patch, try it out, and if it seems good to them, mark it Ready For Checkin - that'll bump it higher on the commit list. But really I just ran short of time the last month or so and never got back to this.

I've done a review now and noticed some new things. I'm not particularly concerned about r12402 - if there was an important reason to forbid this, it should have been explicitly mentioned in that commit message. It seems to me that it simply wasn't considered as a real use case. I am, however, concerned about API consistency, and currently there are two ways this patch breaks it:

  1. If you can raise ValidationError with a dict from Model.clean() and have ModelForm take that up into its error dict, you should be able to do the same from ModelForm.clean() or Form.clean(); in other words, regular form validation and model validation should behave the same in terms of how ValidationError can be used. Having it possible to use a dict from the one clean method and not the others is arbitrary and confusing.
  1. You can instantiate ValidationError with either a single message, or a list of messages. If we're going to add instantiation with a dictionary as a documented API (which seems fine to me), I think the values in that dictionary should similarly be allowed to be either a single message or a list of messages, not required to be a list.

comment:15 in reply to: ↑ 14 ; follow-up: Changed 4 years ago by davidfstr

Replying to carljm:

regular form validation and model validation should behave the same in terms of how ValidationError can be used

OK. I'll investigate making this work for ModelForm.clean() and Form.clean().

Replying to carljm:

I think the values in that dictionary should similarly be allowed to be either a single message or a list of messages, not required to be a list.

My approach here would be to accept such a dictionary as input from the constructor and then normalize it internally to always be a dictionary of message-lists. This means that the dictionary obtained from a ValidationError would not necessarily be equivalent to the dictionary passed in its constructor. I don't see this as a problem in practice, and it avoids having normalization code called in multiple places.

comment:16 in reply to: ↑ 15 Changed 4 years ago by carljm

Replying to davidfstr:

OK. I'll investigate making this work for ModelForm.clean() and Form.clean().

Thanks!

Replying to carljm:

I think the values in that dictionary should similarly be allowed to be either a single message or a list of messages, not required to be a list.

My approach here would be to accept such a dictionary as input from the constructor and then normalize it internally to always be a dictionary of message-lists. This means that the dictionary obtained from a ValidationError would not necessarily be equivalent to the dictionary passed in its constructor. I don't see this as a problem in practice, and it avoids having normalization code called in multiple places.

Fully agreed.

Changed 4 years ago by davidfstr

comment:17 Changed 4 years ago by davidfstr

  • Owner changed from davidfstr to nobody

Not sure if I'll be able to complete this ticket in a reasonable time (i.e. before the end of Dec 2011) so I am ceding ownership.

In case any readers want a workaround before I'm finished, I've attached my own mixin class.

comment:18 Changed 4 years ago by kmike

  • Cc kmike84@… added

comment:19 Changed 3 years ago by gp

  • Cc gp added

comment:20 Changed 2 years ago by jpichon

  • Cc julie@… added

comment:21 Changed 2 years ago by jaylett

  • Cc james@… added

comment:22 Changed 2 years ago by marky1991

  • Cc marky1991 added

comment:23 Changed 2 years ago by loic84

  • Cc loic@… added
  • Patch needs improvement unset

PR with test: https://github.com/django/django/pull/1441.

I believe this has been fixed by the ValidationError refactor from #20199.

As demonstrated in the PR, the syntax is: raise ValidationError({'fieldname': [ValidationError('Error message.')]}).

Passing a dict to the constructor of ValidationError is something we've done internally but that was never documented. We do document passing a list in https://docs.djangoproject.com/en/dev/ref/forms/validation/#raising-multiple-errors. Should we document this, or should it remain an internal thing?

comment:24 Changed 2 years ago by Loic Bistuer <loic.bistuer@…>

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

In 71093d22b62548fc2667468f1ae9e28f4fef30db:

Fixed #16986 -- Model.clean() can report errors on individual fields.

This commit provides the tests for this issue but the actual problem was solved
by the ValidationError refactor in f34cfec and ee77d4b.

Refs #20199.

comment:25 Changed 2 years ago by Marc Tamlyn <marc.tamlyn@…>

In 0b771fcf2923cef1b0d759fda79907c39ad733b4:

Merge pull request #1441 from loic/ticket16986

Fixed #16986 -- Model.clean() can report errors on individual fields.

comment:26 Changed 21 months ago by Loic Bistuer <loic.bistuer@…>

In f563c339ca2eed81706ab17726c79a6f00d7c553:

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

Refs #20199 #16986.

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

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