Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#24988 closed Cleanup/optimization (fixed)

Document raising a dictionary of ValidationErrors

Reported by: Michael Barr Owned by: Tim Graham <timograham@…>
Component: Documentation Version: 1.8
Severity: Normal Keywords: ValidationError
Cc: Loic Bistuer Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description (last modified by Michael Barr)

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):
        """Start date must come before end date."""
        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 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?

Attachments (1)

24988-test.diff (1.2 KB) - added by Tim Graham 4 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 4 years ago by Michael Barr

Description: modified (diff)

comment:2 Changed 4 years ago by Michael Barr

Description: modified (diff)

comment:3 Changed 4 years ago by Tim Graham

Cc: Loic Bistuer added

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.

comment:4 Changed 4 years ago by Michael Barr

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 Raising ValidationError documentation, but can be found as an example in the Model.clean() documentation.

comment:5 Changed 4 years ago by Loic Bistuer

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 Raising ValidationError if only for the sake of exhaustivity.

comment:6 Changed 4 years ago by Tim Graham

Component: Core (Other)Documentation
Summary: ValidationError fails to set codeDocument raising a dictionary of ValidationErrors
Triage Stage: UnreviewedAccepted
Type: BugCleanup/optimization

I'll attach the diff of my investigation, but it looks like this should be a documentation ticket then.

Changed 4 years ago by Tim Graham

Attachment: 24988-test.diff added

comment:7 Changed 4 years ago by Adam Brenecki

Owner: changed from nobody to Adam Brenecki
Status: newassigned

comment:8 Changed 4 years ago by Adam Brenecki

Has patch: set

Would something like this do the job?
https://github.com/django/django/pull/5088

comment:9 Changed 4 years ago by Tim Graham

Patch needs improvement: set

comment:10 Changed 4 years ago by Adam Brenecki

Patch needs improvement: unset

I've removed the 'for instance' bit, is that all that needs fixing?

comment:11 Changed 4 years ago by Tim Graham

Patch needs improvement: set

Loic's comment on the PR makes me think it should be moved to the Model.clean() docs?

comment:12 Changed 4 years ago by Adam Brenecki

Owner: Adam Brenecki deleted
Status: assignednew

comment:13 Changed 4 years ago by Tim Graham <timograham@…>

Owner: set to Tim Graham <timograham@…>
Resolution: fixed
Status: newclosed

In 52a190b:

Fixed #24988 -- Documented passing a dictionary of ValidationErrors to ValidationError

comment:14 Changed 4 years ago by Tim Graham <timograham@…>

In 99b5649a:

[1.8.x] Fixed #24988 -- Documented passing a dictionary of ValidationErrors to ValidationError

Backport of 52a190b65781f8dc07abd230aaf9043fbdbf4fba from master

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