[Django] #35603: "string" in F("...") hangs

[Django]

#35603: "string" in F("...") hangs
-------------------------------------+-------------------------------------
Reporter: Tim Graham | Type: Bug
Status: new | Component: Database
| layer (models, ORM)
Version: 5.1 | 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
-------------------------------------+-------------------------------------
Prior to 94b6f101f7dc363a8e71593570b17527dbb9f77f, this test passes:
{{{#!diff
diff --git a/tests/expressions/tests.py b/tests/expressions/tests.py
index cbb441601c..0600e04e06 100644
--- a/tests/expressions/tests.py
+++ b/tests/expressions/tests.py
@@ -1292,6 +1292,10 @@ class FTests(SimpleTestCase):
self.assertNotEqual(f, value)
self.assertNotEqual(value, f)

+ def test_in(self):
+ with self.assertRaisesMessage(TypeError, "argument of type 'F' is
not iterable"):
+ "foo" in F("name")
+

class ExpressionsTests(TestCase):
def test_F_reuse(self):
}}}

Afterward, it hangs indefinitely.
--
Ticket URL: <https://code.djangoproject.com/ticket/35603>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#35603: "string" in F("...") hangs
-------------------------------------+-------------------------------------

Reporter: Tim Graham | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: 5.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 Simon Charette):

* stage: Unreviewed => Accepted

Comment:

The issue lies in implementing `__getitem__` without implementing
`__contains__`, per
[https://docs.python.org/3/reference/datamodel.html#object.__contains__
the Python docs]

> For objects that don’t define `__contains__()`, the membership test
first tries iteration via `__iter__()`, **then the old sequence iteration
protocol via `__getitem__()`, see
[https://docs.python.org/3/reference/expressions.html#membership-test-
details this section in the language reference]**.

Basically what `"foo" in F("name")` translates to is the following ''old-
style iteration protocol''

> Lastly, the old-style iteration protocol is tried: if a class defines
`__getitem__()`, `x in y` is `True` if and only if there is a non-negative
integer index `i` such that `x is y[i] or x == y[i]`, and no lower integer
index raises the `IndexError` exception.

{{{#!python
def __contains__(self, value):
index = 0
while True:
try:
item = ref.__getitem__(index)
except IndexError:
return False
if item is value or item == value:
return True
index += 1
}}}

which explains why it hangs indefinitely.
--
Ticket URL: <https://code.djangoproject.com/ticket/35603#comment:1>

[Django]

#35603: "string" in F("...") hangs
-------------------------------------+-------------------------------------

Reporter: Tim Graham | Owner: Lufafa
| Joshua
Type: Bug | Status: assigned

Component: Database layer | Version: 5.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 Lufafa Joshua):

* owner: (none) => Lufafa Joshua
* status: new => assigned

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

[Django]

#35603: "string" in F("...") hangs
-------------------------------------+-------------------------------------
Reporter: Tim Graham | Owner: Lufafa
| Joshua
Type: Bug | Status: assigned
Component: Database layer | Version: 5.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 Tim Graham):

Simon, do you think it's better to return False than the old behavior of
raising an exception saying that `F` isn't iterable? The code where I
encountered this is:
{{{#!python
if LOOKUP_SEP in self.query.order_by:
raise NotSupportedError("Ordering can't span tables.")
}}}
This naive implementation doesn't account for `F` objects in ordering. If
we change the behavior to return `False`, my code will crash a bit later.
That doesn't matter since this problem needs to be fixed eventually, but I
tend to think continuing to raise an exception in `F.__contains__()` would
be fine since it doesn't have any meaning.
--
Ticket URL: <https://code.djangoproject.com/ticket/35603#comment:3>

[Django]

#35603: "string" in F("...") hangs
-------------------------------------+-------------------------------------
Reporter: Tim Graham | Owner: Lufafa
| Joshua
Type: Bug | Status: assigned
Component: Database layer | Version: 5.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 Simon Charette):

Tim, given we don't actually support it I think raising a `TypeError`
might effectively be more appropriate. I tried skimming the Python docs to
understand if doing so would be against any established ''protocol'' and I
couldn't find anyway.

To make sure we're aligned, you were thinking of something like

{{{#!diff
diff --git a/django/db/models/expressions.py
b/django/db/models/expressions.py
index dcba973ff4..1d16781959 100644
--- a/django/db/models/expressions.py
+++ b/django/db/models/expressions.py
@@ -884,6 +884,11 @@ def __repr__(self):
def __getitem__(self, subscript):
return Sliced(self, subscript)

+ def __contains__(self, other):
+ # Disable old-style iteration protocol inherited from
implementing
+ # `__getitem__` for slicing to prevent containment checks from
hanging.
+ raise TypeError(f"argument of type '{self.__class__.__name__}' is
not iterable")
+
def resolve_expression(
self, query=None, allow_joins=True, reuse=None, summarize=False,
for_save=False
):
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35603#comment:4>

[Django]

#35603: "string" in F("...") hangs
-------------------------------------+-------------------------------------
Reporter: Tim Graham | Owner: Lufafa
| Joshua
Type: Bug | Status: assigned
Component: Database layer | Version: 5.1
(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 Tim Graham):

* has_patch: 0 => 1
* stage: Accepted => Ready for checkin

Comment:

[https://github.com/django/django/pull/18380 PR]. LGTM!
--
Ticket URL: <https://code.djangoproject.com/ticket/35603#comment:5>

[Django]

#35603: "string" in F("...") hangs
-------------------------------------+-------------------------------------

Reporter: Tim Graham | Owner: Tim
| Graham

Type: Bug | Status: assigned
Component: Database layer | Version: 5.1
(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 Simon Charette):

* owner: Lufafa Joshua => Tim Graham

Comment:

Lufafa, we appreciate your interesting in contributing code to Django but
please ask before jumping on assigning tickets to yourself when an active
discussion is taking place.
--
Ticket URL: <https://code.djangoproject.com/ticket/35603#comment:6>