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

[Zain Memon]

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.

[Zain Memon]

Failing test case demonstrating the conditions for this bug.

[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

[Clément Nodet]

Example patch, including zain original regression test.

[Clément Nodet]

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.

[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.

[Russell Keith-Magee]

Not critical for 1.2

[Ramiro Morales]

Patch with fix by clement and simplified tests

[patchhammer]

10977.diff fails to apply cleanly on to trunk

[Philippe Raoult]

test only vs trunk

[Philippe Raoult]

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.

[Jacob]

Milestone 1.3 deleted