Composite primary key in Case statement evades right-hand side sanity checking

[Jacob Walls]

As ​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 ​suggestion to consolidate this logic in BaseExpression, but I'm not certain of the level of effort there.

tests/composite_pk/test_filter.py

@@ -1 @@
-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 +71 @@
         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")]:

---

======================================================================
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)

[Simon Charette]

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.

[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

django/db/models/expressions.py

@@ -1686 +1686 @@
     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[:]

[Simon Charette]

Early results of refactoring of each expression subclass not calling into super() is ​looking promising.