Re: [Django] #33996: Inconsistent behavior of CheckConstraints validation on None values.
Django
-------------------------------------+-------------------------------------
Reporter: James Beith | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.1
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by David Sanders):
* cc: David Sanders (added)
Comment:
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 >= 0
}}}
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/33996#comment:2>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: James Beith | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.1
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by David Sanders):
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)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33996#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: James Beith | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.1
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by David Sanders):
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/compare/main...shangxiao:django:ticket_33996
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/33996#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: James Beith | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.1
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Simon Charette (added)
Comment:
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
[https://github.com/django/django/pull/14625#pullrequestreview-911482741
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/33996#comment:5>
Django
-------------------------------------+-------------------------------------
| Sanders
Type: Bug | Status: assigned
Component: Database layer | Version: 4.1
(models, ORM) |
Severity: Release blocker | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => David Sanders
* status: new => assigned
* has_patch: 0 => 1
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/33996#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: James Beith | Owner: David
| Sanders
Component: Database layer | Version: 4.1
(models, ORM) |
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"e14d08cd894e9d91cb5d9f44ba7532c1a223f458" e14d08cd]:
{{{
#!CommitTicketReference repository=""
revision="e14d08cd894e9d91cb5d9f44ba7532c1a223f458"
Fixed #33996 -- Fixed CheckConstraint validation on NULL values.
Bug in 667105877e6723c6985399803a364848891513cc.
Thanks James Beith for the report.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33996#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: James Beith | Owner: David
| Sanders
Type: Bug | Status: closed
Component: Database layer | Version: 4.1
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"be5e3b46f78d54f8fae5dc255295d771c2becaac" be5e3b46]:
{{{
#!CommitTicketReference repository=""
revision="be5e3b46f78d54f8fae5dc255295d771c2becaac"
[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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33996#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: James Beith | Owner: David
| Sanders
Type: Bug | Status: closed
Component: Database layer | Version: 4.1
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
In [changeset:"36a000858b5eee13b039aaad490c33b037a1dc05" 36a00085]:
{{{#!CommitTicketReference repository=""
revision="36a000858b5eee13b039aaad490c33b037a1dc05"
Refs #33996 -- Updated CheckConstraint validation on NULL values on Oracle
23c+.
Oracle 23c supports comparing boolean expressions.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33996#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: James Beith | Owner: David
| Sanders
Type: Bug | Status: closed
Component: Database layer | Version: 4.1
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
In [changeset:"61a62be313e395ce1265132bfc99f51476fb3c95" 61a62be3]:
{{{#!CommitTicketReference repository=""
revision="61a62be313e395ce1265132bfc99f51476fb3c95"
Fixed #37057 -- Adjusted UniqueConstraint handling of UNKNOWN condition.
When we adjusted UNKNOWN handling for CheckConstraint in refs #33996 we
assumed
that all usage of Q.check would benefit from this approach.
However while CHECK constraints enforcement do ignore conditions involving
NULL
that resolve to UNKNOWN it's not the case for other type of constraints
such as
UNIQUE ones.
Given how UNKNOWN should be treated depends on the callers context it
appears
that a better strategy for COALESCE wrapping is to force them to apply it
if
necessary.
Thanks Drew Shapiro for the report.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33996#comment:10>