[Django] #24706: ModelForm._post_clean assumes that setting attributes onto an instance cannot raise ValidationError
Django
cannot raise ValidationError
--------------------------------------+--------------------
Reporter: kezabelle | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Forms | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+--------------------
Currently,
[https://github.com/django/django/blob/master/django/forms/models.py#L433
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
[https://github.com/kezabelle/django-
strictmodels/blob/master/strictmodels.py#L14 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`.
--
Ticket URL: <https://code.djangoproject.com/ticket/24706>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
cannot raise ValidationError
Reporter: kezabelle | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by kezabelle):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/24706#comment:1>
Django
cannot raise ValidationError
Reporter: kezabelle | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Forms | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/24706#comment:2>
Django
cannot raise ValidationError
Reporter: kezabelle | Owner: kezabelle
Type: | Status: assigned
Cleanup/optimization |
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by kezabelle):
* owner: nobody => kezabelle
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/24706#comment:3>
Django
cannot raise ValidationError
Reporter: kezabelle | Owner: kezabelle
Type: | Status: assigned
Cleanup/optimization |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by kezabelle):
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/4585 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/24706#comment:4>
Django
cannot raise ValidationError
Reporter: kezabelle | Owner: kezabelle
Type: | Status: assigned
Cleanup/optimization |
Severity: Normal | Resolution:
Keywords: | 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 proposed an alternate implementation on the pull request and it needs
a rebase.
--
Ticket URL: <https://code.djangoproject.com/ticket/24706#comment:5>
Django
cannot raise ValidationError
Reporter: kezabelle | Owner: kezabelle
Type: | Status: assigned
Cleanup/optimization |
Severity: Normal | Resolution:
Keywords: 1.9 | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* keywords: => 1.9
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/24706#comment:6>
Django
cannot raise ValidationError
Reporter: kezabelle | Owner: kezabelle
Type: | Status: assigned
Cleanup/optimization |
Severity: Normal | Resolution:
Keywords: 1.9 | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/24706#comment:7>
Django
cannot raise ValidationError
Reporter: kezabelle | Owner: kezabelle
Cleanup/optimization |
Component: Forms | Version: master
Severity: Normal | Resolution: fixed
Keywords: 1.9 | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"3c5862ccb0fc77418c3b20c21c6b57c40b483464" 3c5862cc]:
{{{
#!CommitTicketReference repository=""
revision="3c5862ccb0fc77418c3b20c21c6b57c40b483464"
Fixed #24706 -- Made ModelForm._post_clean() handle a ValidationError
raised when constructing the model instance.
Thanks Loïc Bistuer for review and advice.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24706#comment:8>