Code

Opened 3 years ago

Closed 8 months ago

#16423 closed Cleanup/optimization (fixed)

ModelForm._post_clean() does not respect Model.clean() ValidationErrors that possess a message_dict, rather than a list of messages

Reported by: robboyle Owned by: nobody
Component: Forms Version: 1.3
Severity: Normal Keywords: modelform, validation
Cc: chris+django@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

When a ModelForm runs it's model-level validation, it anticipates the Model's clean_fields() method raising a ValidationError with the message_dict property set. This let's the ModelForm associate the Model's Field validation errors with the ModelForm fields.

However, when the ModelForm then calls the Model's clean() method, it ignores the message_dict property if the ValidationError raised by clean() has it set. Instead, it always creates a new dictionary, associating all validation messages with the NON_FIELD_ERRORS key.

It would be nice if _post_clean() checked if Model.clean() was raising a ValidationError with a message_dict, and if so updated the errors with that dict, rather than blindly creating a new one. That way, the Model.clean() method could associate errors with a particular field, return a ValidationError with message_dict set, and this association would be perserved by the ModelForm.

Being able to have a form's clean method associate errors with a particular field is very handy. Being able to do this at the model level and then have model forms take advantage of it would bring model forms more on par with regular forms.

Attachments (1)

16423.diff (618 bytes) - added by robboyle 3 years ago.
My initial shot at a solution.

Download all attachments as: .zip

Change History (5)

Changed 3 years ago by robboyle

My initial shot at a solution.

comment:1 Changed 3 years ago by aaugustin

  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Design decision needed

comment:2 Changed 3 years ago by jacob

  • Triage Stage changed from Design decision needed to Accepted

comment:3 Changed 2 years ago by gcc

  • Cc chris+django@… added

+1.

Model.full_clean does this, which is even simpler than the current patch:

        # Form.clean() is run even if other validation fails, so do the
        # same with Model.clean() for consistency.
        try:
            self.clean()
        except ValidationError, e:
            errors = e.update_error_dict(errors)

You could do exactly the same in BaseModelForm.post_clean for symmetry:

        # Call the model instance's clean method.
        try:
            self.instance.clean()
        except ValidationError, e:
            self._update_errors(e.update_error_dict({}))

BaseModelForm._post_clean does almost the same thing as Model.full_clean, and could perhaps be simplified to just call it. The main difference seems to be that validating uniqueness is optional:

        # Validate uniqueness if needed.
        if self._validate_unique:
            self.validate_unique()

and would fail if self.instance.validate_unique threw a ValidationError with a NON_FIELD_ERROR, because it assumes that the ValidationError contains a message_dict:

        try:
            self.instance.validate_unique(exclude=exclude)
        except ValidationError, e:
            self._update_errors(e.message_dict)

comment:4 Changed 8 months ago by timo

  • Resolution set to fixed
  • Status changed from new to closed

I believe this was addressed by [f34cfec0].

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.