Opened 15 years ago

Closed 13 years ago

Last modified 12 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 15 years ago.
Failing test case demonstrating the conditions for this bug.
10977_patch.diff (4.3 KB ) - added by Clément Nodet 15 years ago.
Example patch, including zain original regression test.
10977.diff (2.9 KB ) - added by Ramiro Morales 13 years ago.
Patch with fix by clement and simplified tests
10977_test.diff (965 bytes ) - added by Philippe Raoult 13 years ago.
test only vs trunk

Download all attachments as: .zip

Change History (15)

by Zain Memon, 15 years ago

Attachment: 10977_testcase.diff added

Failing test case demonstrating the conditions for this bug.

comment:1 by Clément Nodet, 15 years ago

Owner: changed from nobody to Clément Nodet

comment:2 by Clément Nodet, 15 years ago

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

by Clément Nodet, 15 years ago

Attachment: 10977_patch.diff added

Example patch, including zain original regression test.

comment:3 by Clément Nodet, 15 years ago

Has patch: set
Owner: Clément Nodet removed

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 by Russell Keith-Magee, 15 years ago

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 by Alex Gaynor, 15 years ago

Triage Stage: UnreviewedAccepted

comment:6 by anonymous, 14 years ago

milestone: 1.2

comment:7 by Russell Keith-Magee, 14 years ago

milestone: 1.21.3

Not critical for 1.2

by Ramiro Morales, 13 years ago

Attachment: 10977.diff added

Patch with fix by clement and simplified tests

comment:8 by patchhammer, 13 years ago

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

10977.diff fails to apply cleanly on to trunk

comment:9 by Julien Phalip, 13 years ago

Type: UncategorizedBug

by Philippe Raoult, 13 years ago

Attachment: 10977_test.diff added

test only vs trunk

comment:10 by Philippe Raoult, 13 years ago

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 by Jacob, 12 years ago

milestone: 1.3

Milestone 1.3 deleted

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