Thanks for your response, Florian. > > modelform.is_valid() fails to anticipate database integrity errors > > when those errors involve any fields that are not part of that form > > itself. > > That is wanted behaviour, eg consider my workflow: > > class SomeForm(ModelForm): > class Meta: > exclude = ['user'] > model = … > > Now in the view I do this: > if form.is_valid(): > instance = form.save(commit=False) > instance.user = request.user
After reading the thread that you referenced, I agree with you that it would be *incorrect* to individually validate fields that are excluded from the form in the `is_valid()` model validation, especially in light of this common idiom (which I do use myself). I think that I described my problem too generically above; nonetheless, I do believe that there is *still* something wrong with the current implementation. I have modified the subject of this thread to reflect real source of my complaint, which is described in the last paragraph of my original ticket: > 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 in the current > implementation 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. If I understood the gist of the model-validation thread you referenced, it is (1) that users of a form should not be presented with a validation error that they are unable to fix by modifying submission data, and (2) that developers should get directly involved if there is any error being generated at form submission time that is out of the control of the user. In other words, it is a GOOD thing for IntegrityError to be raised if instance data that is excluded from the form violates a constraint. I think that the argument is convincing, and I agree with all of these sentiments. However, in the case of a tuple of fields that are "unique together", the proper behavior should be that if *any* of those fields are editable in the form, the constraint should be validated by is_valid(). In the current implementation, *all* of the unique- together fields have to be editable for the constraint to be validated; THAT is the real bug here. It is always fully within the power of a user to resolve a "unique together" constraint violation simply by modifying any one of the fields subject to that constraint; the only thing required is that the user be told which editable field(s) are causing the constraint violation. Furthermore, such violations are not uncommon, and should not require the intervention of the developer. Note that this is not just a problem for me, but is also a problem at the root of several tickets that have already been accepted, several over a year old. These two open tickets pertain to admin and content types, and have the same root cause as I am describing above: http://code.djangoproject.com/ticket/12028 http://code.djangoproject.com/ticket/13091 also: http://code.djangoproject.com/ticket/13249 http://code.djangoproject.com/ticket/15326 Something to take note of regarding #13091, which seems to be the canonical ticket: the current patch attached to this ticket would eliminate *all* exclusions from the call to the validate_unique() model validations. This I now disagree with (as I am sure you do, too), although I originally proposed doing just that. I also think that in the case of a "unique together" tuple where *none* of the fields are editable, model validation should be skipped inside is_valid(). However, I do think that a modification is warranted to resolve the underlying error of the above-listed tickets, as well as my own. > Btw, just my humble opinion: If you exclude fields from the ModelForm > it's your duty to check for valid data. I think this is partly true. However, I believe that with respect to `unique_together`, you should relax this standard. In the case of content-types and generic fields, it is very common to exclude the content-type and object-id fields form the forms, but to also group the content-type and object-id fields with some other editable field in defining a "unique_together" constraint. In such a case, requiring a developer to write boilerplate code to validate uniqueness seems inconvenient, counterintuitive and kind of difficult, actually (IMHO, of course). Furthermore, without adding a whole bunch of complex special-case code to the admin, the current conflict between "list_editable" and "unique_together" is only solvable with the kind of change I am proposing be made to model validation. -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
