Opened 2 years ago

Closed 2 years ago

#19616 closed Bug (needsinfo)

QuerySet.__contains__ tries to check the length of a None

Reported by: kalugny@… Owned by: nobody
Component: Database layer (models, ORM) Version: 1.4
Severity: Normal Keywords:
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description (last modified by lukeplant)

This is the code from QuerySet (version 1.42).
As you can see by the lines I've bolded, although _result_cache might be None, it is still being checked for len()

def __contains__(self, val):
        # The 'in' operator works without this method, due to __iter__. This
        # implementation exists only to shortcut the creation of Model
        # instances, by bailing out early if we find a matching element.
        pos = 0
        if self._result_cache is not None:
            if val in self._result_cache:
                return True
            elif self._iter is None:
                # iterator is exhausted, so we have our answer
                return False
            # remember not to check these again:
            pos = len(self._result_cache)
        else:
            # We need to start filling the result cache out. The following
            # ensures that self._iter is not None and self._result_cache is not
            # None
            it = iter(self)

        # Carry on, one result at a time.
        while True:
           if len(self._result_cache) <= pos:
                self._fill_cache(num=1)

Change History (4)

comment:1 follow-up: Changed 2 years ago by Alex

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Can you actually reproduce this in any way? Calling iter(self) should cause _result_cache to start being a list.

comment:2 in reply to: ↑ 1 Changed 2 years ago by anonymous

Replying to Alex:

Can you actually reproduce this in any way? Calling iter(self) should cause _result_cache to start being a list.

Yes, I noticed this because it happened to me.
The reproduction is a bit complex, though.

I used this snippet:
http://djangosnippets.org/snippets/1034/

And it happened when trying to iterate an empty QuerySet.
I naturally thought that this is a bug with me or the snippet, but looking at the code it is pretty obvious that although the function expects that _result_cache might be None it still tries to len() it.

comment:3 Changed 2 years ago by lukeplant

  • Description modified (diff)

Looking at the code and the comments in the else branch, it actually seems very obvious that the function has covered it's bases about whether self._result_cache is None or not. We need either some further analysis as to why it is wrong, or a simple test case that ought to work.

comment:4 Changed 2 years ago by akaariai

  • Resolution set to needsinfo
  • Status changed from new to closed

IMO the current iterator code is too complex, and the complexity doesn't give us much. See #18702.

I think we want a test case or self contained project which shows the problem. It is hard to say what is happening. From reading the code it seems the situation you describe should not happen, yet no doubt it happens for your code.

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