Opened 10 years ago
Closed 9 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: | dev |
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 by , 10 years ago
comment:2 by , 10 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:3 by , 10 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:4 by , 10 years ago
Has patch: | set |
---|
comment:5 by , 9 years ago
Patch needs improvement: | set |
---|
Loic proposed an alternate implementation on the pull request and it needs a rebase.
comment:6 by , 9 years ago
Keywords: | 1.9 added |
---|---|
Patch needs improvement: | unset |
comment:7 by , 9 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
PR