Opened 4 months ago

Closed 4 months ago

Last modified 4 months ago

#35603 closed Bug (fixed)

"string" in F("...") hangs

Reported by: Tim Graham Owned by: Tim Graham
Component: Database layer (models, ORM) Version: 5.1
Severity: Release blocker Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Prior to 94b6f101f7dc363a8e71593570b17527dbb9f77f, this test passes:

  • tests/expressions/tests.py

    diff --git a/tests/expressions/tests.py b/tests/expressions/tests.py
    index cbb441601c..0600e04e06 100644
    a b class FTests(SimpleTestCase):  
    12921292        self.assertNotEqual(f, value)
    12931293        self.assertNotEqual(value, f)
    12941294
     1295    def test_in(self):
     1296        with self.assertRaisesMessage(TypeError, "argument of type 'F' is not iterable"):
     1297            "foo" in F("name")
     1298
    12951299
    12961300class ExpressionsTests(TestCase):
    12971301    def test_F_reuse(self):

Afterward, it hangs indefinitely.

Change History (8)

comment:1 by Simon Charette, 4 months ago

Triage Stage: UnreviewedAccepted

The issue lies in implementing __getitem__ without implementing __contains__, per 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 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.

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

which explains why it hangs indefinitely.

Implementing __contains__ resolves the issue

  • django/db/models/expressions.py

    diff --git a/django/db/models/expressions.py b/django/db/models/expressions.py
    index dcba973ff4..0e4d259fa3 100644
    a b def __repr__(self):  
    884884    def __getitem__(self, subscript):
    885885        return Sliced(self, subscript)
    886886
     887    def __contains__(self, other):
     888        return False
     889
    887890    def resolve_expression(
    888891        self, query=None, allow_joins=True, reuse=None, summarize=False, for_save=False
    889892    ):
Last edited 4 months ago by Simon Charette (previous) (diff)

comment:2 by Lufafa Joshua, 4 months ago

Owner: set to Lufafa Joshua
Status: newassigned

comment:3 by Tim Graham, 4 months ago

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:

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.

comment:4 by Simon Charette, 4 months ago

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 anything we'd be in violation of.

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

  • django/db/models/expressions.py

    diff --git a/django/db/models/expressions.py b/django/db/models/expressions.py
    index dcba973ff4..1d16781959 100644
    a b def __repr__(self):  
    884884    def __getitem__(self, subscript):
    885885        return Sliced(self, subscript)
    886886
     887    def __contains__(self, other):
     888        # Disable old-style iteration protocol inherited from implementing
     889        # `__getitem__` for slicing to prevent containment checks from hanging.
     890        raise TypeError(f"argument of type '{self.__class__.__name__}' is not iterable")
     891
    887892    def resolve_expression(
    888893        self, query=None, allow_joins=True, reuse=None, summarize=False, for_save=False
    889894    ):
Last edited 4 months ago by Simon Charette (previous) (diff)

comment:5 by Tim Graham, 4 months ago

Has patch: set
Triage Stage: AcceptedReady for checkin

PR. LGTM!

comment:6 by Simon Charette, 4 months ago

Owner: changed from Lufafa Joshua to Tim Graham

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.

comment:7 by Sarah Boyce <42296566+sarahboyce@…>, 4 months ago

Resolution: fixed
Status: assignedclosed

In 6b3f554:

Fixed #35603 -- Prevented F.contains() from hanging.

Regression in 94b6f101f7dc363a8e71593570b17527dbb9f77f.

comment:8 by Sarah Boyce <42296566+sarahboyce@…>, 4 months ago

In 8fb7d304:

[5.1.x] Fixed #35603 -- Prevented F.contains() from hanging.

Regression in 94b6f101f7dc363a8e71593570b17527dbb9f77f.

Backport of 6b3f55446fdc62bd277903fd188a1781e4d92d29 from main.

Note: See TracTickets for help on using tickets.
Back to Top