Opened 13 years ago

Closed 11 years ago

Last modified 11 years ago

#16986 closed New feature (fixed)

Model.clean cannot report errors on individual fields

Reported by: David Foster Owned by: nobody
Component: Forms Version: dev
Severity: Normal Keywords:
Cc: David Foster, danny.adair@…, kmike84@…, gp, julie@…, james@…, Mark Young, 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 David Foster 13 years ago.
model-clean-errordict-2.diff (2.0 KB ) - added by David Foster 13 years ago.
model-clean-errordict-3.diff (5.3 KB ) - added by David Foster 13 years ago.
model-clean-errordict-4.diff (7.0 KB ) - added by David Foster 13 years ago.
model_clean_mixin.py (2.3 KB ) - added by David Foster 13 years ago.

Download all attachments as: .zip

Change History (31)

by David Foster, 13 years ago

Attachment: model-clean-errordict.diff added

comment:1 by Carl Meyer, 13 years ago

Needs documentation: set
Needs tests: set
Patch needs improvement: set
Triage Stage: UnreviewedAccepted
Type: BugNew 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.

Version 0, edited 13 years ago by Carl Meyer (next)

comment:2 by David Foster, 13 years ago

Cc: David Foster 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 by David Foster, 13 years ago

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.

by David Foster, 13 years ago

in reply to:  3 ; comment:4 by Carl Meyer, 13 years ago

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 by danny.adair@…, 13 years ago

Cc: danny.adair@… added

in reply to:  4 comment:6 by David Foster, 13 years ago

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.

by David Foster, 13 years ago

comment:7 by David Foster, 13 years ago

Needs documentation: unset

Added documentation. All patch components complete. Please review.

by David Foster, 13 years ago

comment:8 by David Foster, 13 years ago

Owner: changed from nobody to David Foster

comment:9 by David Foster, 13 years ago

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 by Karen Tracey, 13 years ago

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.

in reply to:  10 comment:11 by anonymous, 13 years ago

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 by David Foster, 13 years ago

(anonymous poster above is me)

comment:13 by David Foster, 13 years ago

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

in reply to:  13 ; comment:14 by Carl Meyer, 13 years ago

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.

in reply to:  14 ; comment:15 by David Foster, 13 years ago

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.

in reply to:  15 comment:16 by Carl Meyer, 13 years ago

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.

by David Foster, 13 years ago

Attachment: model_clean_mixin.py added

comment:17 by David Foster, 13 years ago

Owner: changed from David Foster 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 by Mikhail Korobov, 13 years ago

Cc: kmike84@… added

comment:19 by gp, 12 years ago

Cc: gp added

comment:20 by Julie Pichon, 12 years ago

Cc: julie@… added

comment:21 by James Aylett, 11 years ago

Cc: james@… added

comment:22 by Mark Young, 11 years ago

Cc: Mark Young added

comment:23 by loic84, 11 years ago

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

Resolution: fixed
Status: newclosed

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 by Marc Tamlyn <marc.tamlyn@…>, 11 years ago

In 0b771fcf2923cef1b0d759fda79907c39ad733b4:

Merge pull request #1441 from loic/ticket16986

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

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

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