modelform.is_valid() and modelform.errors fail to anticipate database integrity errors, allow exceptions to reach the user
|Reported by:||legutierr||Owned by:||nobody|
|Has patch:||no||Needs documentation:||no|
|Needs tests:||no||Patch needs improvement:||no|
modelform.is_valid() fails to anticipate database integrity errors when those errors involve any fields that are not part of that form itself. Technically, this is because the
modelform.validate_unique() method uses the
modelform._get_validation_exclusions() method (which lists any model fields that are not in the form itself) to define the exclusions for the call that is made to the ORM object's
validate_unique() method (see here: http://code.djangoproject.com/browser/django/trunk/django/forms/models.py#L339).
In practical terms this is a bad thing because, in a variety of circumstances,
False is the only thing that will prevent
modelform.save() from being called, and
modelform.save() will, in such a case, raise an
IntegretyError that will not be caught. In my opinion,
modelform.is_valid() should always report that a form is NOT valid if it is certain that a call to save() will raise an exception.
The implementation problem here is either:
modelform._get_validation_exclusions()is too liberal in its exclusions,
- that those liberal exclusions should not be passed at all to
- that the implementation of
instance.validate_unique()is using those exclusions incorrectly.
It seems that the original logic was that model fields that are not part of the form should be excluded from the decision whether to mark a form as invalid. But a form *is* invalid if it cannot be saved to the database, regardless of the reason. Now, an argument can be made to the effect that model fields which are not form fields are not the concern of the form and SHOULD cause an IntegrityError to be raised, but that argument is not entirely relevant:
instance.validate_unique() excludes all validations that reference *any* of the excluded fields, even if multiple-field constraints include fields that are, in fact, part of the form. So, if the user changes a field on a form that combines with another, hidden value to violate a constraint, the user will see a 404 or exception page, instead of a meaningful error message explaining how they can fix their mistake.
For me, this is a problem in the case of "unique_together" fields, where one field is editable on the form, and the other is set at record creation time or in some other programmatic way. It is possible, even likely, that a uniqueness constraint will be violated by a user changing the editable field, causing an IntegrityError to rise to the top of the stack, directly impacting the user. Instead, the user should be told that the data they entered is not sufficiently unique.
Change History (10)
comment:1 Changed 5 years ago by
|Patch needs improvement:||unset|
|Summary:||modelform.is_valid() and modelform.errors fail to anticipate database integrety errors, allows exceptions to reach the user → modelform.is_valid() and modelform.errors fail to anticipate database integrity errors, allows exceptions to reach the user|
comment:2 Changed 5 years ago by
|Summary:||modelform.is_valid() and modelform.errors fail to anticipate database integrity errors, allows exceptions to reach the user → modelform.is_valid() and modelform.errors fail to anticipate database integrity errors, allow exceptions to reach the user|