Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#15860 closed Bug (duplicate)

modelform.is_valid() and modelform.errors fail to anticipate database integrity errors, allow exceptions to reach the user

Reported by: legutierr Owned by: nobody
Component: Forms Version: 1.3
Severity: Normal Keywords:
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX:

Description

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, modelform.is_valid() returning 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:

  1. that modelform._get_validation_exclusions() is too liberal in its exclusions,
  2. that those liberal exclusions should not be passed at all to instance.validate_unique(), or
  3. 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 4 years ago by legutierr

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Summary changed from modelform.is_valid() and modelform.errors fail to anticipate database integrety errors, allows exceptions to reach the user to modelform.is_valid() and modelform.errors fail to anticipate database integrity errors, allows exceptions to reach the user

comment:2 Changed 4 years ago by legutierr

  • Summary changed from modelform.is_valid() and modelform.errors fail to anticipate database integrity errors, allows exceptions to reach the user to modelform.is_valid() and modelform.errors fail to anticipate database integrity errors, allow exceptions to reach the user

comment:3 Changed 4 years ago by julien

  • Resolution set to wontfix
  • Status changed from new to closed

I think this is essentially a duplicate of #13091.

comment:4 Changed 4 years ago by julien

Sorry I meant to close this ticket as "Duplicate", certainly not "wontfix". You can't change the resolution without reopening the ticket first, so I won't bother ;)

comment:5 Changed 4 years ago by kmtracey

  • Easy pickings unset
  • Resolution wontfix deleted
  • Status changed from closed to reopened

comment:6 Changed 4 years ago by kmtracey

  • Resolution set to duplicate
  • Status changed from reopened to closed

comment:7 Changed 4 years ago by carljm

I think wontfix is actually the correct resolution for this bug report. The crux of the matter is "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." This is wrong; it is never certain, because of the existence of modelform.save(commit=False) and code like this (presuming a model with FK to User, and a ModelForm that excludes the user FK):

if form.is_valid():
    obj = form.save(commit=False)
    obj.user = request.user
    obj.save()

This is a common and useful idiom, and it would be broken if ModelForm validation had the semantics suggested here. The actual semantics are that the ModelForm is responsible only for validating the fields included in it, and anything excluded is the responsibility of the application developer to handle appropriately.

comment:8 Changed 4 years ago by anonymous

That may be, but this still sounds to me like a dupe of #13091, which is still open. That's where perhaps the observation about why you don't want to take the existing form's instance's value into account. I think that was an unanswered question on that ticket -- why wouldn't you want to take the existing instance's values into account for these check, and what you have mentioned here may be a reason. I'm not sure what the ultimate resolution should be -- if it is possible to accommodate both needs that would be ideal, but I don't have time at the moment to work out whether it is possible....I just think the discussion of it should move to #13091.

comment:9 Changed 4 years ago by kmtracey

(that was me)

comment:10 Changed 4 years ago by legutierr

The way I described this is too broad. Really, the issue is with regards to the validate_unique exclusions being used improperly when evaluating unique_together. Specifically, the current implementation ignores any unique_together constraints where *any* member field is among the exclusions; the correct implementation should ignore only those unique_together constraints where *all* member fields are amone the exclusions.

#13091 describes the manifestation of this problem with regards to the admin; #12028 describes the manifestation of this problem with regards to contenttypes; the underlying issue is best described in #15326. Given that this issue seems to have come up a number of times in a variety of contexts since model validation was added, I hope very much that this is not "won't fix".

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