Hi! Long time user, first time poster.
I’m investigating an issue I’m having with constraint validation. The addition of constraint checking to model validation in Django 4.1 is great! We want to rely on it wherever possible, rather than writing duplicative validation code. However, I’m finding that we get IntegrityErrors in some situations where I would expect ValidationErrors to be raised (and smoothly handled by the admin). I’d like to improve this situation, and haven’t found a post/ticket that quite addresses this.
Short story, using Django 4.2 I have a model MyModel, and we edit it in the admin. Its admin interface hides certain fields after object creation (via MyModelAdmin.get_fieldsets). Some of these fields are those that are referenced by model CheckConstraints. The issue appears to be that Model.validate_constraints receives a set of field names to exclude, generated by BaseModelForm._get_validation_exclusions. When validate_constraints runs, CheckConstraint.validate tries to check the constraint, fails to do so due to missing fields, but then suppresses that FieldError. This makes it such that the constraint is never checked during validation, and an IntegrityError is thereafter raised when the object tries to save.
I read some of the issues referenced in _get_validation_exclusions. But I don’t fully understand why any fields should be excluded from validate_constraints. If I was cleaning field data in MyModel.full_clean to satisfy a constraint, then if I called Model.full_clean before doing so, it could peremptorily raise an error before the fields were ready, but I’m not doing so. Overall, it is odd to me that constraint validation silently fails, leading to IntegrityErrors getting raised when the form saves.
Right now, my workaround seems to be to define MyModel.validate_constraints and NOT pass exclude to the super() call therein. Is this relatively safe? Am I missing something above? Should the docs discuss this behavior in more detail?
Thanks in advance for any thoughts! I can add a minimal example if it would be helpful.
I think an example would be very helpful - if nothing else it would provide a baseline if anyone wanted to replicate the situation in their environment.
Sure! Create a default Django project and app and use the following. Here is a models.py:
from django.db import models
class MyModel(models.Model):
a = models.IntegerField()
b = models.IntegerField()
# Uncomment this to get ValidationError on ModelAdmin edit
# rather than IntegrityError when changing an existing object.
#
# def validate_constraints(self, exclude=None):
# super().validate_constraints() # no exclude!
class Meta:
constraints = [
models.CheckConstraint(
condition=models.Q(a=models.F('b')),
name='my_check',
)
]
And here is admin.py:
from django.contrib import admin
from .models import MyModel
@admin.register(MyModel)
class QuestionAdmin(admin.ModelAdmin):
fields = ['a', 'b']
def get_fields(self, request, obj=None):
fields = list(self.fields)
if obj and obj.pk:
fields.remove('b')
return fields
This is a bit contrived, but it works (Django 5.1.4 used). Create an object in the admin with a==b, then save it, then edit it to change the one field shown, and you get an IntegrityError. If you uncomment the validate_constraints defined on the model, a nice ValidationError occurs instead. I think this is really a forms + ORM issue, not the admin specifically.
1 Like
Hello @jaredahern!
The short answer is that while constraint validation was relatively recently introduced it is skipped for constraint that reference excluded fields to be coherent with how unique together and unique for date validation has historically worked.
As for why form initiated model validation excludes all models fields not present in the form I believe that it is to account for the fact the some model fields values (that might be part of constraints) might be assigned after validation occurs and that (long ago) a decision was taken to allow this pattern to be used at the cost of requiring manual constraint validation when its the case.
There also the question of what exactly should be displayed to the user when their form submission results in a constraint violation involving fields values they cannot control (b in your case). In the face of this ambiguity the system was designed to perform validation only when it could be sure it had all the information at hand and forcing users of field shadowing and constraints to implement their own validation logic.
Now in your case this validation logic could take many forms but I believe that the most appropriate one would be to have your QuestionAdmin provide a form override to provide a clean method that calls Question._meta.constraints[0].validate directly.
1 Like
Thank you @charettes! Apologies, my follow-up here got lengthy…
Got it. It makes sense that it was implemented to be consistent with the uniqueness behavior. However I’m still not clear on the intent behind passing exclude to either Model.validate_unique or Model.validate_constraints.
Excluding fields from Model.clean_fields makes sense to me, as there may need to be instance-wide cleaning in the subsequent Model.clean. Field exclusion from the other two seems to be for a slightly different purpose though. Validation errors there reflect database level errors that will occur on save, so skipping some checks during validation due to excluded fields just masks future issues. There is probably a use case for partial validation I’m not considering, but I wonder whether passing the same exclude set to every method called by Model.full_clean always makes sense.
Agreed that excluding fields from form-level validation and Model.clean_fields makes sense to me for that reason. But I’m not sure it makes sense for the methods called after Model.clean, which I thought was the last place that field values should be set. Are there other places later along the chain in which that can occur?
Absolutely. I think this ties into where the docs or code might not capture this nuance. Model instance reference | Django documentation | Django indicates “ModelForm uses this argument to exclude fields that aren’t present on your form from being validated since any errors raised could not be corrected by the user.” But in the case of a constraint that touches multiple fields, the fact that b can’t be changed doesn’t mean the user can’t correct the problem (understood that somehow showing the b field value would be relevant in my example).
Yes, I see what you are saying. Rechecking the constraints in full makes sense, although I think it means there will be redundant DB queries. I may still be willing to force validate_unique and validate_constraints to ignore the exclude set instead though, since I think we can avoid changing instance fields later than in Model.clean and specially handle any partial validation needs that arise. That might also address similar issues with inlines or in other places without adding more validation code.
A side question is about concurrency: whether validate_unique and validate_constraints (uniqueness and exclusion) and save are invoked atomically by a form within a transaction, as that is another potential source of IntegrityErrors. Poking around at BaseForm.is_valid and other places, I don’t think so. I think this would probably also require something like having full_clean “tentatively” save the instance, and have save just not abort the transaction. But then the reciprocal can occur, where a different save fails validation due to a “tentative” save that gets aborted. I don’t really see a way to avoid these issues without somehow combining database validation and saving into one operation. But none of this is any worse than Python-level constraint validation. Does that all sound correct?
I do agree that it doesn’t mean the user can’t correct the problem but the system was designed around the premise that if it doesn’t have a strong guarantee the user can correct the problem it should refuse the temptation to guess and delegate this responsibility to the developer. Whether or not this is the right design is debatable but the current design is so ossified in the Django ecosystem that it is unlikely to change at this point.
You are correct and it means that due to the way things are currently designed constraint validation is not well suited for a highly concurrent environment. If you host a website with a large amount of traffic that allows users to sign up by themselves and use a standard model form that performs username validation through validate_unique you’ll most likely have noticed monthly logs about IntegrityError resulting from double form submits.
The only way to salvage the current state of things I could think of is to have database backends turn IntegrityError that are tied to constraint validation failures (this is backend dependent) into ConstraintValidationFailure(IntegrityError) instances with a .constraint_name property to maintain PEP 249 compatiblity while allowing upper level abstrations to turn them back into ValidationError.
Assuming this is doable for all backends it would offer a way to have the admin and model class based views to capture ConstraintValidationFailure raised on save, retrieve the proper validation error message from Model._meta.get_constraint(exc.constraint_name).get_violation_error_message(), and raise / handle a proper ValidationError.
Thank you @charettes for all of your insights. They are much appreciated.
That makes sense. Our use case is definitely a bit unusual. I understand the situation much better now though, and can work around it for our use.
Interesting. Yeah, the combination of forms and model behavior does seem to make this challenging. I’ve always wondered about how to make a DB return more explicit errors - this helps clarify that.
My main project now is using DRF APIs, rather than Django forms. So in that case, perhaps there are ways to make the API save process include model constraint validation, without the need for the deeper changes you propose. Time to investigate that more!