#33996 closed Bug (fixed)
Inconsistent behavior of CheckConstraints validation on None values.
Reported by: | James Beith | Owned by: | David Sanders |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | 4.1 |
Severity: | Release blocker | Keywords: | |
Cc: | Gagaro, David Sanders, Simon Charette | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
It appears that Django and the database validate check constraints differently.
Let's say we have this model.
class Person(models.Model): age = models.PositiveIntegerField(blank=True, null=True) class Meta: constraints = [ models.CheckConstraint(check=Q(age__gte=18), name="age_gte_18") ]
If we try to create a person in the database it will raise an error if we violate the constraint.
>>> Person.objects.create(age=15) IntegrityError: new row for relation "data_person" violates check constraint "age_gte_18" DETAIL: Failing row contains (1, 15).
And if the age is None
then the database will not check the constraint, so the object is saved to the database.
>>> Person.objects.create(age=None) <Person: Person object (2)>
In Django 4.1 we have this new behaviour
Check, unique, and exclusion constraints defined in the Meta.constraints option are now checked during model validation.
So if we do this, we do still get the same behaviour as the database.
>>> person = Person(age=15) >>> person.full_clean() ValidationError: {'__all__': ['Constraint "age_gte_18" is violated.']}
But if the age is None
we now get the validation error, even though the database will happily save the object.
>>> person = Person(age=None) >>> person.full_clean() ValidationError: {'__all__': ['Constraint "age_gte_18" is violated.']} >>> person.save() # successfully saves to the database
Is this the expected behaviour?
Should Django's validation be taking into account if the field's define with null=True
and not raise the validation error (so same behaviour as the database)?
Alternatively, is it that the constraint needs updating to also support null so the Django validation passes? e.g.
models.CheckConstraint(check=Q(age__isnull=True) | Q(age__gte=18), name="age_gte_18")
Note: Tested with PostgreSQL, not sure if different databases treat nulls as not breaking the constraint differently.
Change History (9)
comment:1 by , 2 years ago
Cc: | added |
---|---|
Severity: | Normal → Release blocker |
Summary: | Inconsistent behaviour validating constraints between Django and the database → Inconsistent behavior of CheckConstraints validation on None values. |
Triage Stage: | Unreviewed → Accepted |
comment:2 by , 2 years ago
Cc: | added |
---|
Aha I'd also just noticed this behaviour.
It's due to the check constraint doing the validation in the WHERE clause of a query… a NULL value producing an empty resultset hence failing the validation:
SELECT 1 AS "_check" WHERE NULL >= 18
The original PR for this feature mentions moving to WHERE from SELECT to get around certain issue – which is somewhat unfortunate because using SELECT could've given the opportunity to check the result against false.
My naive suggestion would be to add an |Q(field=None) against nullable fields in the check's query.
comment:3 by , 2 years ago
Another alternative would be to coalesce the filters of the query (or the entire WHERE clause but not sure what behaviour we want when dealing with non-nullable columns?) which might be the simpler choice:
postgres=# SELECT 1 WHERE coalesce((NULL >= 18), 't'); ?column? ---------- 1 (1 row)
comment:4 by , 2 years ago
Not to take away from any recommendation from Gagaro or Simon Charette, but here's a patch + test demonstrating coalesce solving the issue: https://github.com/django/django/pull/16038
Note though this pushes logic back into the SELECT and I haven't an Oracle installation to test with. Solution also naively ignores non-nullable fields.
comment:5 by , 2 years ago
Cc: | added |
---|
Thanks for the report and PR folks, this was definitely overlooked during this feature's development.
I went through the comments on the PR that introduced the feature and it seems we could likely work around the Oracle limitation by using SELECT CASE WHEN
by having the wrapping happen directly in Q.check
instead of trying to be clever about relocating the select_format
/supports_boolean_expr_in_select_clause
as originally attempted. Alternatively
As for non-nullable fields I don't think that it should be the responsibility of Constraint.validate
to account for that, we already got validation logic in place to make sure that null=False
don't allow NULL
and having all constraints associated with a non-nullable field that are validated against a None
value for this field raise the same precondition failed ValidationError
seems confusing.
comment:6 by , 2 years ago
Has patch: | set |
---|---|
Owner: | changed from | to
Status: | new → assigned |
Triage Stage: | Accepted → Ready for checkin |
Thanks for the report! Agreed, this behavior should be consistent.
Bug in 667105877e6723c6985399803a364848891513cc.