Opened 20 months ago

Closed 20 months ago

Last modified 6 weeks ago

#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 Mariusz Felisiak, 20 months ago

Cc: Gagaro added
Severity: NormalRelease blocker
Summary: Inconsistent behaviour validating constraints between Django and the databaseInconsistent behavior of CheckConstraints validation on None values.
Triage Stage: UnreviewedAccepted

Thanks for the report! Agreed, this behavior should be consistent.

Bug in 667105877e6723c6985399803a364848891513cc.

comment:2 by David Sanders, 20 months ago

Cc: David Sanders 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.

Last edited 20 months ago by David Sanders (previous) (diff)

comment:3 by David Sanders, 20 months ago

Another alternative would be to coalesce the filters of the query (or the entire WHERE clause) which might be the simpler choice:

postgres=# SELECT 1 WHERE coalesce((NULL >= 18), 't');
 ?column?
----------
        1
(1 row)
Version 0, edited 20 months ago by David Sanders (next)

comment:4 by David Sanders, 20 months 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.

Last edited 20 months ago by David Sanders (previous) (diff)

comment:5 by Simon Charette, 20 months ago

Cc: Simon Charette 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 Mariusz Felisiak, 20 months ago

Has patch: set
Owner: changed from nobody to David Sanders
Status: newassigned
Triage Stage: AcceptedReady for checkin

comment:7 by Mariusz Felisiak <felisiak.mariusz@…>, 20 months ago

Resolution: fixed
Status: assignedclosed

In e14d08cd:

Fixed #33996 -- Fixed CheckConstraint validation on NULL values.

Bug in 667105877e6723c6985399803a364848891513cc.

Thanks James Beith for the report.

comment:8 by Mariusz Felisiak <felisiak.mariusz@…>, 20 months ago

In be5e3b46:

[4.1.x] Fixed #33996 -- Fixed CheckConstraint validation on NULL values.

Bug in 667105877e6723c6985399803a364848891513cc.

Thanks James Beith for the report.

Backport of e14d08cd894e9d91cb5d9f44ba7532c1a223f458 from main

comment:9 by GitHub <noreply@…>, 6 weeks ago

In 36a00085:

Refs #33996 -- Updated CheckConstraint validation on NULL values on Oracle 23c+.

Oracle 23c supports comparing boolean expressions.

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