Opened 4 years ago

Closed 4 years ago

#24706 closed Cleanup/optimization (fixed)

ModelForm._post_clean assumes that setting attributes onto an instance cannot raise ValidationError

Reported by: Keryn Knight Owned by: Keryn Knight
Component: Forms Version: master
Severity: Normal Keywords: 1.9
Cc: django@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Currently, ModelForm._post_clean calls out to construct_instance which, under the hood, uses setattr(instance, self.name, data), assuming that no cleverness is taking place, so doesn't attempt to catch ValidationError as subsequent lines do (when calling full_clean and validate_unique)

This precludes doing things like using __setattr__ on the model, or replacing raw attribute access with descriptors (which is what I did to encounter this) that do validation checks on assignment rather than remembering to call full_clean everywhere.

As far as I can see, this is the only part of the _post_clean implementation where ValidationError could conceivably be being thrown which isn't already handled.

Without catching the exception, it I think necessitates using a custom ModelForm subclass just to do something like:

class MySubForm(ModelForm):
    def _post_clean(self):
        try:
            super(MySubForm, self)._post_clean()
        except ValidationError as e:
            self._update_errors(e)

to avoid the following stacktrace:

django/forms/forms.py:184: in is_valid
    return self.is_bound and not self.errors
django/forms/forms.py:176: in errors
    self.full_clean()
django/forms/forms.py:394: in full_clean
    self._post_clean()
django/forms/models.py:427: in _post_clean
    self.instance = construct_instance(self, self.instance, opts.fields, construct_instance_exclude)
django/forms/models.py:62: in construct_instance
    f.save_form_data(instance, cleaned_data[f.name])
django/db/models/fields/__init__.py:874: in save_form_data
    setattr(instance, self.name, data)
strictmodels.py:42: in __set__
    new_value = self.field.clean(value=value, model_instance=instance)
django/db/models/fields/__init__.py:589: in clean
    self.run_validators(value)

ValidationError: ['error string', 'etc']

As you can see, the exception is the result of my code, but has broken the implied contract of .is_valid() being that it suppresses and collects ValidationErrors.

Change History (8)

comment:1 Changed 4 years ago by Keryn Knight

comment:2 Changed 4 years ago by Tim Graham

Triage Stage: UnreviewedAccepted

comment:3 Changed 4 years ago by Keryn Knight

Owner: changed from nobody to Keryn Knight
Status: newassigned

comment:4 Changed 4 years ago by Keryn Knight

Has patch: set

comment:5 Changed 4 years ago by Tim Graham

Patch needs improvement: set

Loic proposed an alternate implementation on the pull request and it needs a rebase.

comment:6 Changed 4 years ago by Tim Graham

Keywords: 1.9 added
Patch needs improvement: unset

comment:7 Changed 4 years ago by Tim Graham

Triage Stage: AcceptedReady for checkin

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

Resolution: fixed
Status: assignedclosed

In 3c5862cc:

Fixed #24706 -- Made ModelForm._post_clean() handle a ValidationError raised when constructing the model instance.

Thanks Loïc Bistuer for review and advice.

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