[Django] #24988: ValidationError fails to set code
Django
---------------------------------+-----------------------------
Reporter: michaeljohnbarr | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: 1.8
Severity: Normal | Keywords: ValidationError
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+-----------------------------
Assuming the following model:
{{{
class DateRange(models.Model):
start = models.DateField()
end = models.DateField
def clean(self):
super(DateRange, self).clean()
if self.start and self.end:
if self.start >= self.end:
raise ValidationError(
message={
'start': _(
'{start} must come before {end}.'.format(
start=self._meta.get_field(
'start'
).verbose_name,
end=self._meta.get_field(
'end'
).verbose_name,
)
),
'end': _(
'{end} must come after {start}.'.format(
end=self._meta.get_field(
'end'
).verbose_name,
start=self._meta.get_field(
'start'
).verbose_name,
)
),
},
code='invalid',
)
}}}
After then validating the model with a ModelForm, the `code` fails to be
set to `'invalid'` but is instead an empty string. I discovered this when
I was programming tests:
{{{
def test_start_before_end_validation(self):
"""An employee is not allowed to be their own manager."""
TestForm = modelform_factory(
model=DateRange,
fields=('start', 'end')
)
form = TestForm(
data=dict(
start=date(2015, 1, 1),
end=date(1900, 12, 31) # WHOOPS!
)
)
self.assertEqual(first=form.is_valid(), second=False)
# The below fails because the code is '' - we have to leave off
the code
self.assertTrue(form.has_error(field='start'), code='invalid')
self.assertTrue(form.has_error(field='end'), code='invalid')
}}}
Upon inspection of the the `ValidationError` code, I discovered that that
any `ValidationError` that is raised with a type of `dict` or a `list` as
the `message` will never set the code:
{{{
if isinstance(message, dict):
self.error_dict = {}
for field, messages in message.items():
if not isinstance(messages, ValidationError):
messages = ValidationError(messages)
self.error_dict[field] = messages.error_list
elif isinstance(message, list):
self.error_list = []
for message in message:
# Normalize plain strings to instances of ValidationError.
if not isinstance(message, ValidationError):
message = ValidationError(message)
if hasattr(message, 'error_dict'):
self.error_list.extend(sum(message.error_dict.values(), []))
else:
self.error_list.extend(message.error_list)
}}}
According to the
[https://docs.djangoproject.com/en/1.8/ref/forms/validation/#raising-
validationerror documentation on raising ValidationError], it is suggested
to "Provide a descriptive error code to the constructor." Is this a bug?
I believe the "fix" would be as simple as to pass `code=code` following
the message/messages, but I may be wrong. Thoughts?
--
Ticket URL: <https://code.djangoproject.com/ticket/24988>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: michaeljohnbarr | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: 1.8
Keywords: ValidationError | Triage Stage: Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by michaeljohnbarr):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Old description:
New description:
Assuming the following model:
def clean(self):
super(DateRange, self).clean()
{{{
def test_start_before_end_validation(self):
"""Start date must come before end date."""
self.assertEqual(first=form.is_valid(), second=False)
--
--
Ticket URL: <https://code.djangoproject.com/ticket/24988#comment:1>
Django
Reporter: michaeljohnbarr | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: 1.8
Keywords: ValidationError | Triage Stage: Unreviewed
Easy pickings: 0 | UI/UX: 0
Description changed by michaeljohnbarr:
Old description:
> {{{
> def test_start_before_end_validation(self):
> """Start date must come before end date."""
New description:
Assuming the following model:
{{{
class DateRange(models.Model):
start = models.DateField()
end = models.DateField()
def clean(self):
super(DateRange, self).clean()
{{{
def test_start_before_end_validation(self):
"""Start date must come before end date."""
self.assertEqual(first=form.is_valid(), second=False)
--
--
Ticket URL: <https://code.djangoproject.com/ticket/24988#comment:2>
Django
Reporter: michaeljohnbarr | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: 1.8
Keywords: ValidationError | Triage Stage: Unreviewed
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* cc: loic (added)
Comment:
I think it's preferred to use ValidationErrors as the values of the
messages dict.
{{{
'start': ValidationError_(
'{start} must come before {end}.'
params={...},
code='invalid',
),
'end': ...
}}}
The change you suggested makes sense to me, but I'm not sure if we should
do it. If not, might be helpful to issue a warning or exception rather
than silently discarding the value from the code parameter.
--
Ticket URL: <https://code.djangoproject.com/ticket/24988#comment:3>
Django
Reporter: michaeljohnbarr | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: 1.8
Keywords: ValidationError | Triage Stage: Unreviewed
Easy pickings: 0 | UI/UX: 0
Comment (by michaeljohnbarr):
I can confirm that using ValidationError as the value does work as
expected. Perhaps if we don't change how it works internally, we can
update the documentation. This particular use case is not covered in the
[https://docs.djangoproject.com/en/1.8/ref/forms/validation/#raising-
validationerror Raising ValidationError] documentation, but can be found
as an example in the
[https://docs.djangoproject.com/en/1.8/ref/models/instances/#django.db.models.Model.clean
Model.clean() documentation].
--
Ticket URL: <https://code.djangoproject.com/ticket/24988#comment:4>
Django
Reporter: michaeljohnbarr | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: 1.8
Keywords: ValidationError | Triage Stage: Unreviewed
Easy pickings: 0 | UI/UX: 0
Comment (by loic):
`code` and `params` are really intended for single message
`ValidationError`, if we did pass `code=code` in the constructor we'd be
effectively creating an implicit shortcut, I don't think it's a good idea
(explicit better than implicit, TIMTOWTDI, etc.). Also it's ambiguous if
you pass a `dict` that mixes strings (which will be turned into
`ValidationError`) and `ValidationError` instances.
Raising `ValidationError` with `error_dict` is mostly useful in
`Model.clean()` because forms have better APIs (i.e. `Form.add_error()`),
but I guess we can still document it in
[https://docs.djangoproject.com/en/1.8/ref/forms/validation/#raising-
validationerror Raising ValidationError] if only for the sake of
exhaustivity.
--
Ticket URL: <https://code.djangoproject.com/ticket/24988#comment:5>
Django
--------------------------------------+------------------------------------
Reporter: michaeljohnbarr | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Documentation | Version: 1.8
Severity: Normal | Resolution:
Keywords: ValidationError | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* type: Bug => Cleanup/optimization
* component: Core (Other) => Documentation
* stage: Unreviewed => Accepted
Comment:
I'll attach the diff of my investigation, but it looks like this should be
a documentation ticket then.
--
Ticket URL: <https://code.djangoproject.com/ticket/24988#comment:6>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Component: Documentation | Version: 1.8
Severity: Normal | Resolution:
Keywords: ValidationError | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* Attachment "24988-test.diff" added.
Django
Reporter: michaeljohnbarr | Owner:
Type: | adambrenecki
Cleanup/optimization | Status: assigned
Component: Documentation | Version: 1.8
Severity: Normal | Resolution:
Keywords: ValidationError | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by adambrenecki):
* owner: nobody => adambrenecki
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/24988#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: michaeljohnbarr | Owner:
Type: | adambrenecki
Cleanup/optimization | Status: assigned
Component: Documentation | Version: 1.8
Severity: Normal | Resolution:
Keywords: ValidationError | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by adambrenecki):
* has_patch: 0 => 1
Comment:
Would something like this do the job?
https://github.com/django/django/pull/5088
--
Ticket URL: <https://code.djangoproject.com/ticket/24988#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: michaeljohnbarr | Owner:
Type: | adambrenecki
Cleanup/optimization | Status: assigned
Component: Documentation | Version: 1.8
Severity: Normal | Resolution:
Keywords: ValidationError | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/24988#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: michaeljohnbarr | Owner:
Type: | adambrenecki
Cleanup/optimization | Status: assigned
Component: Documentation | Version: 1.8
Severity: Normal | Resolution:
Keywords: ValidationError | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by adambrenecki):
* needs_better_patch: 1 => 0
Comment:
I've removed the 'for instance' bit, is that all that needs fixing?
--
Ticket URL: <https://code.djangoproject.com/ticket/24988#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: michaeljohnbarr | Owner:
Type: | adambrenecki
Cleanup/optimization | Status: assigned
Component: Documentation | Version: 1.8
Severity: Normal | Resolution:
Keywords: ValidationError | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 0 => 1
Comment:
Loic's comment on the PR makes me think it should be moved to the
`Model.clean()` docs?
--
Ticket URL: <https://code.djangoproject.com/ticket/24988#comment:11>
Django
Reporter: michaeljohnbarr | Owner:
Type: Cleanup/optimization | Status: new
Component: Documentation | Version: 1.8
Severity: Normal | Resolution:
Keywords: ValidationError | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by adambrenecki):
* owner: adambrenecki =>
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/24988#comment:12>
Django
Reporter: michaeljohnbarr | Owner: Tim
Type: | Graham <timograham@…>
Cleanup/optimization | Status: closed
Component: Documentation | Version: 1.8
Severity: Normal | Resolution: fixed
Keywords: ValidationError | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by Tim Graham <timograham@…>):
* owner: => Tim Graham <timograham@…>
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"52a190b65781f8dc07abd230aaf9043fbdbf4fba" 52a190b]:
{{{
#!CommitTicketReference repository=""
revision="52a190b65781f8dc07abd230aaf9043fbdbf4fba"
Fixed #24988 -- Documented passing a dictionary of ValidationErrors to
ValidationError
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24988#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: michaeljohnbarr | Owner: Tim
Type: | Graham <timograham@…>
Cleanup/optimization | Status: closed
Component: Documentation | Version: 1.8
Severity: Normal | Resolution: fixed
Keywords: ValidationError | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by Tim Graham <timograham@…>):
In [changeset:"99b5649a0788fa9e816debf6c987b19157340acf" 99b5649a]:
{{{
#!CommitTicketReference repository=""
revision="99b5649a0788fa9e816debf6c987b19157340acf"
[1.8.x] Fixed #24988 -- Documented passing a dictionary of
ValidationErrors to ValidationError
Backport of 52a190b65781f8dc07abd230aaf9043fbdbf4fba from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24988#comment:14>