[Django] #37291: Passing F("pk") to a tuple lookup can hang

[Django]

#37291: Passing F("pk") to a tuple lookup can hang
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Type: Bug
Status: new | Component: Database
| layer (models, ORM)
Version: 5.2 | Severity: Release
| blocker
Keywords: compositeprimarykey | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
This test provides `F("pk")` directly to the `TupleIn` lookup. It hangs
due to a sanity check in the composite PK logic:

{{{#!py
diff --git a/tests/foreign_object/test_tuple_lookups.py
b/tests/foreign_object/test_tuple_lookups.py
index 008f118994..fb1965fa6a 100644
--- a/tests/foreign_object/test_tuple_lookups.py
+++ b/tests/foreign_object/test_tuple_lookups.py
@@ -168,6 +168,12 @@ class TupleLookupsTests(TestCase):
with self.subTest(customer=customer.id, query=str(qs.query)):
self.assertSequenceEqual(qs, contacts)

+ def test_tuple_in_subquery_f(self):
+ self.assertCountEqual(
+ Contact.objects.filter(TupleIn(F("pk"),
Contact.objects.values("pk"))),
+ Contact.objects.all(),
+ )
+
def test_tuple_in_rhs_must_be_collection_of_tuples_or_lists(self):
test_cases = (
(1, 2, 3),
}}}



That sanity check iterates over the left-hand side expression, which is
apparently not safe if the left-hand side is an `F` object, as when
`__iter__()` missing, Python falls back to `__getitem__()`, and
`F.__getitem__()` can perpetually iterate an index:
{{{#!py
/Users/jwalls/django/tests/foreign_object/test_tuple_lookups.py(173)test_tuple_in_subquery_f()
-> Contact.objects.filter(TupleIn(F("pk"), Contact.objects.values("pk"))),
/Users/jwalls/django/django/db/models/lookups.py(35)__init__()
-> self.rhs = self.get_prep_lookup()
/Users/jwalls/django/django/db/models/fields/tuple_lookups.py(302)get_prep_lookup()
-> self.check_rhs_is_tuple_or_list()
/Users/jwalls/django/django/db/models/fields/tuple_lookups.py(63)check_rhs_is_tuple_or_list()
-> lhs_str = self.get_lhs_str()
/Users/jwalls/django/django/db/models/fields/tuple_lookups.py(89)get_lhs_str()
-> names = ", ".join(repr(f.name) for f in self.lhs)
/Users/jwalls/django/django/db/models/fields/tuple_lookups.py(89)<genexpr>()->"'pk'"
-> names = ", ".join(repr(f.name) for f in self.lhs)
> /Users/jwalls/django/django/db/models/expressions.py(898)__getitem__()
}}}
`F.__getitem__()`:
{{{#!py
def __getitem__(self, subscript):
return Sliced(self, subscript)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/37291>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#37291: Passing F("pk") to a tuple lookup can hang
-------------------------------------+-------------------------------------

Reporter: Jacob Walls | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: compositeprimarykey | Triage Stage: Accepted

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

Changes (by Clifford Gama):

* stage: Unreviewed => Accepted

Comment:

Thanks! I've reproduced the bug
--
Ticket URL: <https://code.djangoproject.com/ticket/37291#comment:1>

[Django]

#37291: Passing F("pk") to a tuple lookup can hang
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: compositeprimarykey | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

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

Changes (by Yassin Bahri):

* has_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/37291#comment:2>

[Django]

#37291: Passing F("pk") to a tuple lookup can hang
-------------------------------------+-------------------------------------

Reporter: Jacob Walls | Owner: Yassin
| Bahri
Type: Bug | Status: assigned

Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: compositeprimarykey | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Yassin Bahri):

* owner: (none) => Yassin Bahri
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/37291#comment:3>

[Django]

#37291: Passing F("pk") to a tuple lookup can hang
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Yassin
| Bahri
Type: Bug | Status: assigned
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: compositeprimarykey | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Yassin Bahri):

A patch is available at https://github.com/django/django/pull/21806.

The change normalizes a directly supplied `QuerySet` into a cloned SQL
`Query` before `TupleIn` validates its right-hand side. This prevents the
lookup from treating the `QuerySet` as a direct value and attempting to
iterate `F("pk")`.

The original `QuerySet` is not mutated, and its database alias is
preserved.

The reported regression test was added, and the complete tuple lookup test
module passes:

{{{
Ran 24 tests in 0.058s

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

[Django]

#37291: Passing F("pk") to a tuple lookup can hang
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Yassin
| Bahri
Type: Bug | Status: assigned
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: compositeprimarykey | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Pravin):

I think the same issue may affect the other tuple lookups as well then
--
Ticket URL: <https://code.djangoproject.com/ticket/37291#comment:5>

[Django]

#37291: Passing F("pk") to a tuple lookup can hang
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Yassin
| Bahri
Type: Bug | Status: assigned
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: compositeprimarykey | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Yassin Bahri):

Replying to [comment:5 Pravin]:

> I think the same issue may affect the other tuple lookups as well then

Yes, thanks. The fix is in `TupleLookupMixin.get_lhs_str()`, which is
shared by the tuple lookup implementations.

I expanded the regression coverage to verify an `F("pk")` left-hand side
with an invalid scalar RHS for:

* `TupleExact`
* `TupleGreaterThan`
* `TupleGreaterThanOrEqual`
* `TupleIn`
* `TupleLessThan`
* `TupleLessThanOrEqual`

Each now raises its expected `ValueError` immediately instead of
attempting to iterate the `F()` expression.

`TupleIsNull` has separate RHS validation and does not use `get_lhs_str()`
on this path.

All 25 tests in `foreign_object.test_tuple_lookups` pass with the expanded
coverage.
--
Ticket URL: <https://code.djangoproject.com/ticket/37291#comment:6>