#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)
Change History (31)
by , 13 years ago
Attachment: | model-clean-errordict.diff added |
---|
comment:1 by , 13 years ago
Needs documentation: | set |
---|---|
Needs tests: | set |
Patch needs improvement: | set |
Triage Stage: | Unreviewed → Accepted |
Type: | Bug → New feature |
comment:2 by , 13 years ago
Cc: | 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.
follow-up: 4 comment:3 by , 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 , 13 years ago
Attachment: | model-clean-errordict-2.diff added |
---|
follow-up: 6 comment:4 by , 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 , 13 years ago
Cc: | added |
---|
comment:6 by , 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. EssentiallyValidationError
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 , 13 years ago
Attachment: | model-clean-errordict-3.diff added |
---|
comment:7 by , 13 years ago
Needs documentation: | unset |
---|
Added documentation. All patch components complete. Please review.
by , 13 years ago
Attachment: | model-clean-errordict-4.diff added |
---|
comment:8 by , 13 years ago
Owner: | changed from | to
---|
comment:9 by , 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/.
follow-up: 11 comment:10 by , 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.
comment:11 by , 13 years ago
This is what I am able to piece together:
- Pre r12402, it was possible to raise a
ValidationError
with a dictionary but not a list of messages. - 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.) - 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.
follow-up: 14 comment:13 by , 13 years ago
Is there any futher action on my part that is preventing this ticket from proceeding?
follow-up: 15 comment:14 by , 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:
- If you can raise
ValidationError
with a dict fromModel.clean()
and haveModelForm
take that up into its error dict, you should be able to do the same fromModelForm.clean()
orForm.clean()
; in other words, regular form validation and model validation should behave the same in terms of howValidationError
can be used. Having it possible to use a dict from the one clean method and not the others is arbitrary and confusing.
- 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.
follow-up: 16 comment:15 by , 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.
comment:16 by , 13 years ago
Replying to davidfstr:
OK. I'll investigate making this work for
ModelForm.clean()
andForm.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 , 13 years ago
Attachment: | model_clean_mixin.py added |
---|
comment:17 by , 13 years ago
Owner: | changed from | to
---|
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 , 13 years ago
Cc: | added |
---|
comment:19 by , 12 years ago
Cc: | added |
---|
comment:20 by , 12 years ago
Cc: | added |
---|
comment:21 by , 11 years ago
Cc: | added |
---|
comment:22 by , 11 years ago
Cc: | added |
---|
comment:23 by , 11 years ago
Cc: | 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 , 11 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
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
fromclean
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 outsideValidationError
, I think the patch should introduce a newhas_dict
property onValidationError
that keeps that check internal to the implementation (and may as well clean up the internalhasattr
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.