[Django] #36117: Composite primary key in Case statement evades right-hand side sanity checking

[Django]

#36117: Composite primary key in Case statement evades right-hand side sanity
checking
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Type: Bug
Status: new | Component: Database
| layer (models, ORM)
Version: 5.2 | Severity: Release
| blocker
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
As [https://github.com/django/django/pull/18979/files#r1903443787
mentioned] on ticket:36042, I figured there might be some more cases where
a right-hand side composite expression sneaks through to the database
layer and fails there with a syntax error. Found one with `Case`:

Suggesting to fail at the ORM layer instead of the db, like the other
sanity checks. This might be a chance to look into Simon's
[https://github.com/django/django/pull/18979/files#r1904377673 suggestion]
to consolidate this logic in `BaseExpression`, but I'm not certain of the
level of effort there.

{{{#!diff
diff --git a/tests/composite_pk/test_filter.py
b/tests/composite_pk/test_filter.py
index 4edf947423..4ff9b5a4ea 100644
--- a/tests/composite_pk/test_filter.py
+++ b/tests/composite_pk/test_filter.py
@@ -1,4 +1,13 @@
-from django.db.models import F, FilteredRelation, OuterRef, Q, Subquery,
TextField
+from django.db.models import (
+ Case,
+ F,
+ FilteredRelation,
+ OuterRef,
+ Q,
+ Subquery,
+ TextField,
+ When,
+)
from django.db.models.functions import Cast
from django.db.models.lookups import Exact
from django.test import TestCase
@@ -62,6 +71,13 @@ class CompositePKFilterTests(TestCase):
with self.assertRaisesMessage(ValueError, msg):
Comment.objects.filter(text__gt=F("pk")).count()

+ def test_rhs_case(self):
+ msg = "CompositePrimaryKey cannot be used as a lookup value."
+ with self.assertRaisesMessage(ValueError, msg):
+ Comment.objects.filter(
+ text=Case(When(text="", then="pk"), default="pk")
+ ).count()
+
def test_rhs_combinable(self):
msg = "CompositePrimaryKey is not combinable."
for expr in [F("pk") + (1, 1), (1, 1) + F("pk")]:
}}}

----
{{{#!py
======================================================================
ERROR: test_rhs_case
(composite_pk.test_filter.CompositePKFilterTests.test_rhs_case)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/Users/source/django/django/db/backends/utils.py", line 105, in
_execute
return self.cursor.execute(sql, params)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/source/django/django/db/backends/sqlite3/base.py", line
360, in execute
return super().execute(query, params)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
sqlite3.OperationalError: near ",": syntax error

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
File "/Users/source/django/tests/composite_pk/test_filter.py", line 79,
in test_rhs_case
).count()
^^^^^^^
File "/Users/source/django/django/db/models/query.py", line 603, in
count
return self.query.get_count(using=self.db)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/source/django/django/db/models/sql/query.py", line 644, in
get_count
return obj.get_aggregation(using, {"__count": Count("*")})["__count"]
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/source/django/django/db/models/sql/query.py", line 626, in
get_aggregation
result = compiler.execute_sql(SINGLE)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/source/django/django/db/models/sql/compiler.py", line 1623,
in execute_sql
cursor.execute(sql, params)
File "/Users/source/django/django/db/backends/utils.py", line 122, in
execute
return super().execute(sql, params)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/source/django/django/db/backends/utils.py", line 79, in
execute
return self._execute_with_wrappers(
^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/source/django/django/db/backends/utils.py", line 92, in
_execute_with_wrappers
return executor(sql, params, many, context)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/source/django/django/db/backends/utils.py", line 100, in
_execute
with self.db.wrap_database_errors:
File "/Users/source/django/django/db/utils.py", line 91, in __exit__
raise dj_exc_value.with_traceback(traceback) from exc_value
File "/Users/source/django/django/db/backends/utils.py", line 105, in
_execute
return self.cursor.execute(sql, params)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/source/django/django/db/backends/sqlite3/base.py", line
360, in execute
return super().execute(query, params)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
django.db.utils.OperationalError: near ",": syntax error

----------------------------------------------------------------------
(0.000) SELECT COUNT(*) AS "__count"
FROM "composite_pk_comment"
WHERE "composite_pk_comment"."text" = (CASE
WHEN
"composite_pk_comment"."text" = '' THEN
"composite_pk_comment"."tenant_id",
"composite_pk_comment"."comment_id"
ELSE
"composite_pk_comment"."tenant_id",
"composite_pk_comment"."comment_id"
END); args=('',); alias=default

----------------------------------------------------------------------
Ran 125 tests in 0.563s

FAILED (errors=1)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36117>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#36117: Composite primary key in Case statement evades right-hand side sanity
checking
-------------------------------------+-------------------------------------

Reporter: Jacob Walls | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: 5.2
(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 Simon Charette):

* stage: Unreviewed => Accepted

Comment:

Thanks Jacob, I'll see if I can get the `resolve_expression` consolidation
to pass tests that would effectively allow us to get a lot more of the
`allows_composite_expressions` solution.
--
Ticket URL: <https://code.djangoproject.com/ticket/36117#comment:1>

[Django]

#36117: Composite primary key in Case statement evades right-hand side sanity
checking
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: 5.2
(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 Simon Charette):

Without implementing the full thing it seems that simply removing the
`Case.resolve_expression` implementation (which is basically equivalent to
`BaseExpression.resolve_expression`) passes the tests

{{{#!diff
diff --git a/django/db/models/expressions.py
b/django/db/models/expressions.py
index 2494ec4139..83ace22086 100644
--- a/django/db/models/expressions.py
+++ b/django/db/models/expressions.py
@@ -1686,20 +1686,6 @@ def get_source_expressions(self):
def set_source_expressions(self, exprs):
*self.cases, self.default = exprs

- def resolve_expression(
- self, query=None, allow_joins=True, reuse=None, summarize=False,
for_save=False
- ):
- c = self.copy()
- c.is_summary = summarize
- for pos, case in enumerate(c.cases):
- c.cases[pos] = case.resolve_expression(
- query, allow_joins, reuse, summarize, for_save
- )
- c.default = c.default.resolve_expression(
- query, allow_joins, reuse, summarize, for_save
- )
- return c
-
def copy(self):
c = super().copy()
c.cases = c.cases[:]
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36117#comment:2>

[Django]

#36117: Composite primary key in Case statement evades right-hand side sanity
checking
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: 5.2
(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 Simon Charette):

* cc: Simon Charette (added)

Comment:

Early results of refactoring of each expression subclass not calling into
`super()` is [https://github.com/django/django/pull/19073 looking
promising].
--
Ticket URL: <https://code.djangoproject.com/ticket/36117#comment:3>

[Django]

#36117: Composite primary key in Case statement evades right-hand side sanity
checking
-------------------------------------+-------------------------------------

Reporter: Jacob Walls | Owner: Simon
| Charette
Type: Bug | Status: assigned

Component: Database layer | Version: 5.2
(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 Jacob Walls):

* owner: (none) => Simon Charette
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/36117#comment:4>

[Django]

#36117: Composite primary key in Case statement evades right-hand side sanity
checking
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Simon Charette):

* has_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/36117#comment:5>

[Django]

#36117: Composite primary key in Case statement evades right-hand side sanity
checking
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Release blocker | Resolution:

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
-------------------------------------+-------------------------------------

Changes (by Sarah Boyce):

* stage: Accepted => Ready for checkin

--
Ticket URL: <https://code.djangoproject.com/ticket/36117#comment:6>

[Django]

#36117: Composite primary key in Case statement evades right-hand side sanity
checking
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Simon
| Charette

Type: Bug | Status: closed

Component: Database layer | Version: 5.2
(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
-------------------------------------+-------------------------------------

Changes (by Sarah Boyce <42296566+sarahboyce@…>):

* resolution: => fixed
* status: assigned => closed

Comment:

In [changeset:"00c690efbc0b10f67924687f24a7b30397bf47d9" 00c690ef]:
{{{#!CommitTicketReference repository=""
revision="00c690efbc0b10f67924687f24a7b30397bf47d9"
Fixed #36117 -- Raised ValueError when providing composite expressions to
case / when.

Remove redundant Case and When.resolve_expression to delegate composite
expression support to BaseExpression.

Thanks Jacob Tyler Walls for the report and test.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36117#comment:7>

[Django]

#36117: Composite primary key in Case statement evades right-hand side sanity
checking
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Simon
| Charette
Type: Bug | Status: closed
Component: Database layer | Version: 5.2
(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 Sarah Boyce <42296566+sarahboyce@…>):

In [changeset:"1df0f998ae8d1626adaa9b807a43e4cbdfc590e2" 1df0f99]:
{{{#!CommitTicketReference repository=""
revision="1df0f998ae8d1626adaa9b807a43e4cbdfc590e2"
[5.2.x] Fixed #36117 -- Raised ValueError when providing composite

expressions to case / when.

Remove redundant Case and When.resolve_expression to delegate composite
expression support to BaseExpression.

Thanks Jacob Tyler Walls for the report and test.

Backport of 00c690efbc0b10f67924687f24a7b30397bf47d9 from main.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36117#comment:8>