Opened 6 years ago

Closed 4 years ago

Last modified 4 years ago

#10977 closed Bug (fixed)

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

Reported by: zain 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 6 years ago.
Failing test case demonstrating the conditions for this bug.
10977_patch.diff (4.3 KB) - added by clement 6 years ago.
Example patch, including zain original regression test.
10977.diff (2.9 KB) - added by ramiro 4 years ago.
Patch with fix by clement and simplified tests
10977_test.diff (965 bytes) - added by PhiR 4 years ago.
test only vs trunk

Download all attachments as: .zip

Change History (15)

Changed 6 years ago by zain

Failing test case demonstrating the conditions for this bug.

comment:1 Changed 6 years ago by clement

  • Needs documentation unset
  • Needs tests unset
  • Owner changed from nobody to clement
  • Patch needs improvement unset

comment:2 Changed 6 years ago by clement

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 6 years ago by clement

Example patch, including zain original regression test.

comment:3 Changed 6 years ago by clement

  • Has patch set
  • Owner clement 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 6 years ago by russellm

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 6 years ago by Alex

  • Triage Stage changed from Unreviewed to Accepted

comment:6 Changed 5 years ago by anonymous

  • milestone set to 1.2

comment:7 Changed 5 years ago by russellm

  • milestone changed from 1.2 to 1.3

Not critical for 1.2

Changed 4 years ago by ramiro

Patch with fix by clement and simplified tests

comment:8 Changed 4 years ago by patchhammer

  • Easy pickings unset
  • Patch needs improvement set
  • Severity set to Normal
  • Type set to Uncategorized

10977.diff fails to apply cleanly on to trunk

comment:9 Changed 4 years ago by julien

  • Type changed from Uncategorized to Bug

Changed 4 years ago by PhiR

test only vs trunk

comment:10 Changed 4 years ago by PhiR

  • Resolution set to fixed
  • Status changed from new to closed
  • 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 4 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

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