Fixed #36050 -- Added OuterRef support to CompositePrimaryKey. #18994

csirmazbendeguz · 2025-01-03T09:59:00Z

Trac ticket number

ticket-36050

Branch description

Use OuterRef with CompositePrimaryKey

Checklist

This PR targets the main branch.
The commit message is written in past tense, mentions the ticket number, and ends with a period.
I have checked the "Has patch" ticket flag in the Trac system.
I have added or updated relevant tests.
I have added or updated relevant docs, including release notes if applicable.
I have attached screenshots in both light and dark modes for any UI changes.

tests/composite_pk/test_filter.py

jacobtylerwalls

Thanks for your quick attention to this!

I found a case that fails with FilteredRelation. I don't know what we would want to do to guard against this or if this is a follow-up issue, but it's new as of this branch:

diff --git a/tests/composite_pk/test_filter.py b/tests/composite_pk/test_filter.py
index b553118468..9e1f78cdd2 100644
--- a/tests/composite_pk/test_filter.py
+++ b/tests/composite_pk/test_filter.py
@@ -1,8 +1,8 @@
-from django.db.models import F, OuterRef, Subquery
+from django.db.models import F, FilteredRelation, OuterRef, Q, Subquery
 from django.db.models.lookups import Exact
 from django.test import TestCase
 
-from .models import Comment, Tenant, User
+from .models import Comment, Tenant, Token, User
 
 
 class CompositePKFilterTests(TestCase):
@@ -38,6 +38,7 @@ class CompositePKFilterTests(TestCase):
         cls.comment_3 = Comment.objects.create(id=3, user=cls.user_2)
         cls.comment_4 = Comment.objects.create(id=4, user=cls.user_3)
         cls.comment_5 = Comment.objects.create(id=5, user=cls.user_1)
+        cls.token_1 = Token.objects.create(id=1, tenant=cls.tenant_1)
 
     def test_filter_and_count_user_by_pk(self):
         test_cases = (
@@ -56,6 +57,17 @@ class CompositePKFilterTests(TestCase):
             with self.subTest(lookup=lookup, count=count):
                 self.assertEqual(User.objects.filter(**lookup).count(), count)
 
+    def test_pk_filtered_relation(self):
+        self.assertSequenceEqual(
+            Tenant.objects.annotate(
+                filtered_tokens=FilteredRelation(
+                    # correctly(?) raises orm-level error with tokens__gte=
+                    "tokens", condition=Q(tokens__pk__gte=OuterRef("tokens"))
+                )
+            ).filter(filtered_tokens=(1, 1)),
+            [self.tenant_1],
+        )
+
     def test_order_comments_by_pk_asc(self):
         self.assertSequenceEqual(
             Comment.objects.order_by("pk"),

  File "/Users/.../django/django/db/backends/sqlite3/base.py", line 360, in execute
    return super().execute(query, params)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
django.db.utils.OperationalError: no such column: T3.tenant_id

----------------------------------------------------------------------
(0.000)
SELECT "composite_pk_tenant"."id",
       "composite_pk_tenant"."name"
FROM "composite_pk_tenant"
INNER JOIN "composite_pk_token" filtered_tokens ON ("composite_pk_tenant"."id" = filtered_tokens."tenant_id"
                                                    AND ((filtered_tokens."tenant_id",
                                                          filtered_tokens."id") >= (T3."tenant_id",
                                                                                    T3."id")))
WHERE (filtered_tokens."tenant_id",
       filtered_tokens."id") = (1,
                                1);

args=(1,
      1);

ALIAS=DEFAULT
----------------------------------------------------------------------
Ran 64 tests in 0.423s

FAILED (errors=1, skipped=1)

csirmazbendeguz · 2025-01-04T07:16:09Z

thanks @jacobtylerwalls ! your reviews are very helpful

OuterRef in FilteredRelation is not possible and this example should be raising the error

This queryset contains a reference to an outer query and may only be used in a subquery.

I'll need to check why that's not happening

sarahboyce · 2025-01-08T14:23:07Z

This needs some updates since 6eec703 was merged 🙏

sarahboyce

Thank you!

charettes · 2025-01-12T22:50:51Z

tests/composite_pk/test_filter.py

+
+        msg = "Composite field lookups only work with composite expressions."
+        with self.assertRaisesMessage(ValueError, msg):
+            self.assertEqual(queryset.count(), 5)


This test crashes on Oracle

====================================================================== ERROR: test_outer_ref_not_composite_pk (composite_pk.test_filter.CompositePKFilterTests) ---------------------------------------------------------------------- Traceback (most recent call last): File "/django/source/tests/composite_pk/test_filter.py", line 442, in test_outer_ref_not_composite_pk self.assertEqual(queryset.count(), 5) File "/django/source/django/db/models/query.py", line 604, in count return self.query.get_count(using=self.db) File "/django/source/django/db/models/sql/query.py", line 644, in get_count return obj.get_aggregation(using, {"__count": Count("*")})["__count"] File "/django/source/django/db/models/sql/query.py", line 626, in get_aggregation result = compiler.execute_sql(SINGLE) File "/django/source/django/db/models/sql/compiler.py", line 1611, in execute_sql sql, params = self.as_sql() File "/django/source/django/db/models/sql/compiler.py", line 795, in as_sql self.compile(self.where) if self.where is not None else ("", []) File "/django/source/django/db/models/sql/compiler.py", line 578, in compile sql, params = node.as_sql(self, self.connection) File "/django/source/django/db/models/sql/where.py", line 151, in as_sql sql, params = compiler.compile(child) File "/django/source/django/db/models/sql/compiler.py", line 576, in compile sql, params = vendor_impl(self, self.connection) File "/django/source/django/db/models/lookups.py", line 155, in as_oracle return lookup.as_sql(compiler, connection) File "/django/source/django/db/models/lookups.py", line 393, in as_sql return super().as_sql(compiler, connection) File "/django/source/django/db/models/lookups.py", line 250, in as_sql rhs_sql, rhs_params = self.process_rhs(compiler, connection) File "/django/source/django/db/models/lookups.py", line 443, in process_rhs return super().process_rhs(compiler, connection) File "/django/source/django/db/models/lookups.py", line 121, in process_rhs sql, params = compiler.compile(value) File "/django/source/django/db/models/sql/compiler.py", line 578, in compile sql, params = node.as_sql(self, self.connection) File "/django/source/django/db/models/expressions.py", line 1795, in as_sql subquery_sql, sql_params = self.query.as_sql(compiler, connection) File "/django/source/django/db/models/sql/query.py", line 1284, in as_sql sql, params = self.get_compiler(connection=connection).as_sql() File "/django/source/django/db/models/sql/compiler.py", line 795, in as_sql self.compile(self.where) if self.where is not None else ("", []) File "/django/source/django/db/models/sql/compiler.py", line 578, in compile sql, params = node.as_sql(self, self.connection) File "/django/source/django/db/models/sql/where.py", line 151, in as_sql sql, params = compiler.compile(child) File "/django/source/django/db/models/sql/compiler.py", line 576, in compile sql, params = vendor_impl(self, self.connection) File "/django/source/django/db/models/fields/tuple_lookups.py", line 104, in as_oracle lookups = [Exact(col, val) for col, val in zip(self.lhs, self.rhs)] TypeError: 'Col' object is not iterable

charettes · 2025-01-12T22:51:36Z

tests/composite_pk/test_filter.py

+            "This queryset contains a reference to an outer query and may only be used "
+            "in a subquery."
+        )
+        with self.assertRaisesMessage(ValueError, msg):


This test causes a recursion error on Oracle; it's at the origin of the broken CI

https://djangoci.com/job/django-oracle/1468/

Sorry! I forgot to test with Oracle... it would be wise to run Oracle tests as part of the regular test suite. Why are we not doing that?

No worries @csirmazbendeguz! I agree that it would be great if were able to systematically run Oracle tests but this is unfortunately not feasible because it's so much slower than other backends to run the suite at ~1h40 minutes.

We've had discussions with Oracle in the past for them to provide some instance that could be used for CI purpose but it never materialized. I have hope that the recent changes in https://github.com/django/django-docker-box will bring us closer to a point where we can run things on Github actions but in the mean time the rule of thumb I use to invoke Oracle tests is whether or not any aspect touches SQL compilation. When it's the case I systematically invoke the Oracle suite.

charettes

It appears the change here are only implemented for backends that support native tuple lookups. In other words, they are broken on Oracle and all the as_oracle implementations will crash.

Issues is addressed by #19035.

csirmazbendeguz force-pushed the ticket_36050 branch from f45fd0c to 6998f6c Compare January 3, 2025 10:01

csirmazbendeguz changed the title ~~Fixed #36050 -- Added OuterRef support to CompositePrimaryKey~~ Fixed #36050 -- Added OuterRef support to CompositePrimaryKey. Jan 3, 2025

csirmazbendeguz force-pushed the ticket_36050 branch 4 times, most recently from 598a6f3 to c02d266 Compare January 3, 2025 11:06

felixxm reviewed Jan 3, 2025

View reviewed changes

tests/composite_pk/test_filter.py Outdated Show resolved Hide resolved

tests/composite_pk/test_filter.py Outdated Show resolved Hide resolved

tests/composite_pk/test_filter.py Outdated Show resolved Hide resolved

jacobtylerwalls reviewed Jan 3, 2025

View reviewed changes

csirmazbendeguz force-pushed the ticket_36050 branch 5 times, most recently from e27b01a to b24a134 Compare January 7, 2025 12:11

sarahboyce force-pushed the ticket_36050 branch from b24a134 to edc4b47 Compare January 8, 2025 14:05

csirmazbendeguz force-pushed the ticket_36050 branch 2 times, most recently from 8cf15e8 to 34577fb Compare January 9, 2025 12:34

Fixed #36050 -- Added OuterRef support to CompositePrimaryKey.

a936c09

sarahboyce force-pushed the ticket_36050 branch from 34577fb to a936c09 Compare January 10, 2025 09:38

sarahboyce approved these changes Jan 10, 2025

View reviewed changes

sarahboyce merged commit 8bee7fa into django:main Jan 10, 2025
42 checks passed

charettes reviewed Jan 12, 2025

View reviewed changes

This was referenced Jan 12, 2025

Refs #36050 -- Fixed OuterRef support for CompositePrimaryKey on Oracle. #19035

Merged

Fixed #36088 -- Avoided unnecessary DEFAULT usage on bulk_create(). #18909

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Fixed #36050 -- Added OuterRef support to CompositePrimaryKey. #18994

Fixed #36050 -- Added OuterRef support to CompositePrimaryKey. #18994

Uh oh!

csirmazbendeguz commented Jan 3, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jacobtylerwalls left a comment

Uh oh!

csirmazbendeguz commented Jan 4, 2025

Uh oh!

sarahboyce commented Jan 8, 2025

Uh oh!

sarahboyce left a comment

Uh oh!

Uh oh!

charettes Jan 12, 2025

Uh oh!

charettes Jan 12, 2025

Uh oh!

csirmazbendeguz Jan 13, 2025

Uh oh!

charettes Jan 13, 2025

Uh oh!

charettes left a comment •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Fixed #36050 -- Added OuterRef support to CompositePrimaryKey. #18994

Fixed #36050 -- Added OuterRef support to CompositePrimaryKey. #18994

Uh oh!

Conversation

csirmazbendeguz commented Jan 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Trac ticket number

Branch description

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jacobtylerwalls left a comment

Choose a reason for hiding this comment

Uh oh!

csirmazbendeguz commented Jan 4, 2025

Uh oh!

sarahboyce commented Jan 8, 2025

Uh oh!

sarahboyce left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

charettes Jan 12, 2025

Choose a reason for hiding this comment

Uh oh!

charettes Jan 12, 2025

Choose a reason for hiding this comment

Uh oh!

csirmazbendeguz Jan 13, 2025

Choose a reason for hiding this comment

Uh oh!

charettes Jan 13, 2025

Choose a reason for hiding this comment

Uh oh!

charettes left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

csirmazbendeguz commented Jan 3, 2025 •

edited

Loading

charettes left a comment •

edited

Loading