Model.clean cannot report errors on individual fields

[David Foster]

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.

[Carl Meyer]

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.

[David Foster]

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

[David Foster]

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.

[Carl Meyer]

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.

[David Foster]

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.

[David Foster]

Added documentation. All patch components complete. Please review.

[David Foster]

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

[Karen Tracey]

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.

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

[David Foster]

(anonymous poster above is me)

[David Foster]

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

[Carl Meyer]

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.

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

[David Foster]

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.

[Carl Meyer]

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.

[David Foster]

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.

[loic84]

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?

[Loic Bistuer <loic.bistuer@…>]

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.

[Marc Tamlyn <marc.tamlyn@…>]

In 0b771fcf2923cef1b0d759fda79907c39ad733b4:

Merge pull request #1441 from loic/ticket16986

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

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