#27434 closed Cleanup/optimization (fixed)
Document caveats of raising a ValidationError in Model.clean() for a field not in a model form
Reported by: | Matthias Kestenholz | Owned by: | Matthias Kestenholz |
---|---|---|---|
Component: | Documentation | Version: | dev |
Severity: | Normal | Keywords: | |
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
Example:
class DictionaryValidationErrorModel(models.Model): field1 = models.CharField(max_length=100) field2 = models.CharField(max_length=100) field3 = models.CharField(max_length=100) def clean(self): super(DictionaryValidationErrorModel, self).clean() raise ValidationError({ 'field1': 'field1 error', 'field2': 'field2 error', })
class DictionaryValidationErrorTests(ValidationTestCase): def test_default_form(self): class ModelForm(forms.ModelForm): class Meta: model = DictionaryValidationErrorModel fields = ('field1', 'field2', 'field3') form = ModelForm({ 'field1': '1', 'field2': '2', 'field3': '3', }) self.assertFalse(form.is_valid()) self.assertEqual(set(form.errors.keys()), {'field1', 'field2'}) def test_crash(self): class ModelForm(forms.ModelForm): class Meta: model = DictionaryValidationErrorModel fields = ('field1', 'field3') form = ModelForm({ 'field1': '1', 'field2': '2', 'field3': '3', }) self.assertFalse(form.is_valid()) self.assertEqual(set(form.errors.keys()), {'field1', 'field2'})
The problem I'm seeing is
====================================================================== ERROR: test_crash (validation.test_validators.DictionaryValidationErrorTests) ---------------------------------------------------------------------- Traceback (most recent call last): File "/usr/lib/python3.5/unittest/case.py", line 59, in testPartExecutor yield File "/usr/lib/python3.5/unittest/case.py", line 601, in run testMethod() File "/home/matthias/Projects/django/tests/validation/test_validators.py", line 62, in test_crash self.assertFalse(form.is_valid()) File "/home/matthias/Projects/django/django/forms/forms.py", line 169, in is_valid return self.is_bound and not self.errors File "/home/matthias/Projects/django/django/forms/forms.py", line 161, in errors self.full_clean() File "/home/matthias/Projects/django/django/forms/forms.py", line 372, in full_clean self._post_clean() File "/home/matthias/Projects/django/django/forms/models.py", line 400, in _post_clean self._update_errors(e) File "/home/matthias/Projects/django/django/forms/models.py", line 374, in _update_errors self.add_error(None, errors) File "/home/matthias/Projects/django/django/forms/forms.py", line 338, in add_error "'%s' has no field named '%s'." % (self.__class__.__name__, field)) ValueError: 'ModelForm' has no field named 'field2'.
This might be a thing for me to work on during the sprints. Also, https://github.com/matthiask/django/commit/2d03c38f888262e63c179b57c0c6bd09312f6f99
(Cc:ing Loïc because we discussed this at DUTH on wednesday evening.)
Change History (9)
comment:1 by , 8 years ago
Has patch: | set |
---|---|
Owner: | changed from | to
Status: | new → assigned |
comment:2 by , 8 years ago
Triage Stage: | Unreviewed → Accepted |
---|---|
Version: | 1.10 → master |
comment:3 by , 8 years ago
Patch needs improvement: | set |
---|
comment:4 by , 8 years ago
Patch needs improvement: | unset |
---|
I hope the patch won't need further improvement, removing the flag for now. Thanks.
comment:7 by , 8 years ago
Component: | Forms → Documentation |
---|---|
Patch needs improvement: | set |
Summary: | Crashes during form validation when Model validation raises ValidationErrors for fields not in the current form → Document caveats of raising a ValidationError in Model.clean() for a field not in a model form |
Triage Stage: | Ready for checkin → Accepted |
Type: | Bug → Cleanup/optimization |
I left a few ideas for improvement on the PR.
Note:
See TracTickets
for help on using tickets.
I'm still not sure if we should discard the error or move it to
NON_FIELD_ERRORS
as suggested here but it shouldn't be crashing.PR