Opened 8 years ago

Closed 5 years ago

Last modified 5 years ago

#10977 closed Bug (fixed)

In some cases, the intersection of two QuerySets is empty when it shouldn't be

Reported by: Zain Memon Owned by:
Component: Database layer (models, ORM) Version: 1.0
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

This bug is best demonstrated by an example with two querysets, qs and other_qs:

>>> qs
[<Recommendation: Foo>, <Recommendation: Bar>]
>>> other_qs
[<Recommendation: Foo>]
>>> qs[0] == other_qs[0]
True
>>> qs & other_qs
[]

I'm attaching a test case to demonstrate the conditions for this to happen.

Attachments (4)

10977_testcase.diff (3.5 KB) - added by Zain Memon 8 years ago.
Failing test case demonstrating the conditions for this bug.
10977_patch.diff (4.3 KB) - added by Clément Nodet 8 years ago.
Example patch, including zain original regression test.
10977.diff (2.9 KB) - added by Ramiro Morales 6 years ago.
Patch with fix by clement and simplified tests
10977_test.diff (965 bytes) - added by Philippe Raoult 5 years ago.
test only vs trunk

Download all attachments as: .zip

Change History (15)

Changed 8 years ago by Zain Memon

Attachment: 10977_testcase.diff added

Failing test case demonstrating the conditions for this bug.

comment:1 Changed 8 years ago by Clément Nodet

Owner: changed from nobody to Clément Nodet

comment:2 Changed 8 years ago by Clément Nodet

Well, it seems there's a bug in BaseQuery.combine, when relabeling aliases. It happens when the rhs query object contains JOIN from an aliased table; they are converted to JOINs from the original table because the code is building a new join using data coming from .rev_join_map (and not .alias_map) :

# django/db/models/sql/query.py, line 501
        for alias in rhs.tables:
            if not rhs.alias_refcount[alias]:
                # An unused alias.
                continue
            promote = (rhs.alias_map[alias][JOIN_TYPE] == self.LOUTER)
            new_alias = self.join(rhs.rev_join_map[alias],
                    (conjunction and not first), used, promote, not conjunction)
            used.add(new_alias)
            change_map[alias] = new_alias

To recreate this bug, any query joining from an alias will be enough, and will give an incorrect SQL query string when combined (right-operand) to a simple query, on the same base object. For example, with :

class ModelA(models.Model):
  text = models.CharField(max_length=100)
  a = models.ForeignKey('ModelA')
  b = models.ForeignKey('ModelB')

class ModelB(models.Model):
  text = models.CharField(max_length=100)

That will give:

>>> qs_simple = ModelA.objects.all()
>>> qs_joined = ModelA.objects.filter(b__text__icontains='foo').filter(a__b__text__icontains='bar')
>>> str(qs_simple.query) # Formatted for clarity
SELECT "queries_modela"."id", "queries_modela"."text", "queries_modela"."a_id", "queries_modela"."b_id"
    FROM "queries_modela"
>>> str(qs_joined.query) # Formatted for clarity
SELECT "queries_modela"."id", "queries_modela"."text", "queries_modela"."a_id", "queries_modela"."b_id"
    FROM "queries_modela"
    INNER JOIN "queries_modelb"
        ON ("queries_modela"."b_id" = "queries_modelb"."id")
    INNER JOIN "queries_modela" T3
        ON ("queries_modela"."a_id" = T3."id")
    INNER JOIN "queries_modelb" T4
        ON (T3."b_id" = T4."id")
WHERE ("queries_modelb"."text" LIKE %foo% ESCAPE '\' AND T4."text" LIKE %bar% ESCAPE '\' )
>>> str((qs_simple & qs_joined).query) # Formatted for clarity
SELECT "queries_modela"."id", "queries_modela"."text", "queries_modela"."a_id", "queries_modela"."b_id"
    FROM "queries_modela"
    INNER JOIN "queries_modelb"
        ON ("queries_modela"."b_id" = "queries_modelb"."id")
    INNER JOIN "queries_modela" T3
        ON ("queries_modela"."a_id" = T3."id")
    INNER JOIN "queries_modelb" T4
        ON ("queries_modela"."b_id" = T4."id")
WHERE ("queries_modelb"."text" LIKE %foo% ESCAPE '\'  AND T4."text" LIKE %bar% ESCAPE '\' )

Seeing here that the T4 join ON condition becomes incorrect when qs_joined is combined to qs_simple

Changed 8 years ago by Clément Nodet

Attachment: 10977_patch.diff added

Example patch, including zain original regression test.

comment:3 Changed 8 years ago by Clément Nodet

Has patch: set
Owner: Clément Nodet deleted

The attached fix does the following in BaseQuery.combine() :

  • get the source of the join from alias_map[alias][LHS_ALIAS]
  • check if there's a corresponding newly labelled alias in change_map dict, and use it in that case
  • send that source as the first element of the connection tuple to BaseQuery.join() (the rest of the tuple being what is in rev_join_map[alias])

I've ran the full test suite (./runtests.py) against this patch, and it passes, but this being my first shot at Django's internals, I don't have a lot of insight on how large the impact of that patch might be.

comment:4 Changed 7 years ago by Russell Keith-Magee

Ok - a request was made to push this into v1.1. To me. the fix looks ok (although I would probably avoid using a variable name like "connection" since it's a word that is already in use), and if the full Django test suite still passes after applying the fix, then that's about all the confirmation you need that the fix is correct (or at least, no more incorrect).

The biggest thing preventing me from committing this is that that the test case is trying to be far too clever. The original report gives a very clear example of some queries, but no setup. The test case is - ironically - longer than the original report, and is doing all sorts of convoluted tricks with manually instantiated QuerySets, calls to reduce(), and dynamically rolled out Q() objects. What's wrong with reproducing the original problem case? It should be easy to understand what a test is trying to achieve - in this case, the test is more complex than the problem it's trying to solve.

comment:5 Changed 7 years ago by Alex Gaynor

Triage Stage: UnreviewedAccepted

comment:6 Changed 7 years ago by anonymous

milestone: 1.2

comment:7 Changed 7 years ago by Russell Keith-Magee

milestone: 1.21.3

Not critical for 1.2

Changed 6 years ago by Ramiro Morales

Attachment: 10977.diff added

Patch with fix by clement and simplified tests

comment:8 Changed 6 years ago by patchhammer

Easy pickings: unset
Patch needs improvement: set
Severity: Normal
Type: Uncategorized

10977.diff fails to apply cleanly on to trunk

comment:9 Changed 6 years ago by Julien Phalip

Type: UncategorizedBug

Changed 5 years ago by Philippe Raoult

Attachment: 10977_test.diff added

test only vs trunk

comment:10 Changed 5 years ago by Philippe Raoult

Resolution: fixed
Status: newclosed
UI/UX: unset

This bug looked suspiciously similar to #12252, so I ran the tests vs trunk and it seems that the issue is no longer showing up. For reference I have attached the updated test.

comment:11 Changed 5 years ago by Jacob

milestone: 1.3

Milestone 1.3 deleted

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