Skip to content
This repository

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse code

Fixed #20564 -- Generic relations exclude() regression

The patch for #19385 caused a regression in certain generic relations
.exclude() filters if a subquery was needed. The fix contains a
refactoring to how Query.split_exclude() and Query.trim_start()
interact.

Thanks to Trac alias nferrari for the report.
  • Loading branch information...
commit 31fd64ad8a98d7de0acb9144ae6f7bd124700cb0 1 parent 8c5b805
Anssi Kääriäinen authored June 06, 2013
105  django/db/models/sql/query.py
@@ -1422,7 +1422,9 @@ def split_exclude(self, filter_expr, prefix, can_reuse, names_with_path):
1422 1422
         query.clear_ordering(True)
1423 1423
         # Try to have as simple as possible subquery -> trim leading joins from
1424 1424
         # the subquery.
1425  
-        trimmed_joins = query.trim_start(names_with_path)
  1425
+        trimmed_prefix, contains_louter = query.trim_start(names_with_path)
  1426
+        query.remove_inherited_models()
  1427
+
1426 1428
         # Add extra check to make sure the selected field will not be null
1427 1429
         # since we are adding a IN <subquery> clause. This prevents the
1428 1430
         # database from tripping over IN (...,NULL,...) selects and returning
@@ -1431,38 +1433,20 @@ def split_exclude(self, filter_expr, prefix, can_reuse, names_with_path):
1431 1433
             alias, col = query.select[0].col
1432 1434
             query.where.add((Constraint(alias, col, query.select[0].field), 'isnull', False), AND)
1433 1435
 
1434  
-        # Still make sure that the trimmed parts in the inner query and
1435  
-        # trimmed prefix are in sync. So, use the trimmed_joins to make sure
1436  
-        # as many path elements are in the prefix as there were trimmed joins.
1437  
-        # In addition, convert the path elements back to names so that
1438  
-        # add_filter() can handle them.
1439  
-        trimmed_prefix = []
1440  
-        paths_in_prefix = trimmed_joins
1441  
-        for name, path in names_with_path:
1442  
-            if paths_in_prefix - len(path) < 0:
1443  
-                break
1444  
-            trimmed_prefix.append(name)
1445  
-            paths_in_prefix -= len(path)
1446  
-        join_field = path[paths_in_prefix].join_field
1447  
-        # TODO: This should be made properly multicolumn
1448  
-        # join aware. It is likely better to not use build_filter
1449  
-        # at all, instead construct joins up to the correct point,
1450  
-        # then construct the needed equality constraint manually,
1451  
-        # or maybe using SubqueryConstraint would work, too.
1452  
-        # The foreign_related_fields attribute is right here, we
1453  
-        # don't ever split joins for direct case.
1454  
-        trimmed_prefix.append(
1455  
-            join_field.field.foreign_related_fields[0].name)
1456  
-        trimmed_prefix = LOOKUP_SEP.join(trimmed_prefix)
1457 1436
         condition = self.build_filter(
1458 1437
             ('%s__in' % trimmed_prefix, query),
1459 1438
             current_negated=True, branch_negated=True, can_reuse=can_reuse)
1460  
-        # Intentionally leave the other alias as blank, if the condition
1461  
-        # refers it, things will break here.
1462  
-        extra_restriction = join_field.get_extra_restriction(
1463  
-            self.where_class, None, [t for t in query.tables if query.alias_refcount[t]][0])
1464  
-        if extra_restriction:
1465  
-            query.where.add(extra_restriction, 'AND')
  1439
+        if contains_louter:
  1440
+            or_null_condition = self.build_filter(
  1441
+                ('%s__isnull' % trimmed_prefix, True),
  1442
+                current_negated=True, branch_negated=True, can_reuse=can_reuse)
  1443
+            condition.add(or_null_condition, OR)
  1444
+            # Note that the end result will be:
  1445
+            # (outercol NOT IN innerq AND outercol IS NOT NULL) OR outercol IS NULL.
  1446
+            # This might look crazy but due to how IN works, this seems to be
  1447
+            # correct. If the IS NOT NULL check is removed then outercol NOT
  1448
+            # IN will return UNKNOWN. If the IS NULL check is removed, then if
  1449
+            # outercol IS NULL we will not match the row.
1466 1450
         return condition
1467 1451
 
1468 1452
     def set_empty(self):
@@ -1821,35 +1805,58 @@ def _extra_select(self):
1821 1805
     def trim_start(self, names_with_path):
1822 1806
         """
1823 1807
         Trims joins from the start of the join path. The candidates for trim
1824  
-        are the PathInfos in names_with_path structure. Outer joins are not
1825  
-        eligible for removal. Also sets the select column so the start
1826  
-        matches the join.
  1808
+        are the PathInfos in names_with_path structure that are m2m joins.
  1809
+
  1810
+        Also sets the select column so the start matches the join.
  1811
+
  1812
+        This method is meant to be used for generating the subquery joins &
  1813
+        cols in split_exclude().
1827 1814
 
1828  
-        This method is mostly useful for generating the subquery joins & col
1829  
-        in "WHERE somecol IN (subquery)". This construct is needed by
1830  
-        split_exclude().
  1815
+        Returns a lookup usable for doing outerq.filter(lookup=self). Returns
  1816
+        also if the joins in the prefix contain a LEFT OUTER join.
1831 1817
         _"""
1832 1818
         all_paths = []
1833 1819
         for _, paths in names_with_path:
1834 1820
             all_paths.extend(paths)
1835  
-        direct_join = True
  1821
+        contains_louter = False
1836 1822
         for pos, path in enumerate(all_paths):
  1823
+            if path.m2m:
  1824
+                break
1837 1825
             if self.alias_map[self.tables[pos + 1]].join_type == self.LOUTER:
1838  
-                direct_join = False
1839  
-                pos -= 1
  1826
+                contains_louter = True
  1827
+            self.unref_alias(self.tables[pos])
  1828
+        # The path.join_field is a Rel, lets get the other side's field
  1829
+        join_field = path.join_field.field
  1830
+        # Build the filter prefix.
  1831
+        trimmed_prefix = []
  1832
+        paths_in_prefix = pos
  1833
+        for name, path in names_with_path:
  1834
+            if paths_in_prefix - len(path) < 0:
1840 1835
                 break
  1836
+            trimmed_prefix.append(name)
  1837
+            paths_in_prefix -= len(path)
  1838
+        trimmed_prefix.append(
  1839
+            join_field.foreign_related_fields[0].name)
  1840
+        trimmed_prefix = LOOKUP_SEP.join(trimmed_prefix)
  1841
+        # Lets still see if we can trim the first join from the inner query
  1842
+        # (that is, self). We can't do this for LEFT JOINs because we would
  1843
+        # miss those rows that have nothing on the outer side.
  1844
+        if self.alias_map[self.tables[pos + 1]].join_type != self.LOUTER:
  1845
+            select_fields = [r[0] for r in join_field.related_fields]
  1846
+            select_alias = self.tables[pos + 1]
1841 1847
             self.unref_alias(self.tables[pos])
1842  
-        if path.direct:
1843  
-            direct_join = not direct_join
1844  
-        join_side = 0 if direct_join else 1
1845  
-        select_alias = self.tables[pos + 1]
1846  
-        join_field = path.join_field
1847  
-        if hasattr(join_field, 'field'):
1848  
-            join_field = join_field.field
1849  
-        select_fields = [r[join_side] for r in join_field.related_fields]
  1848
+            extra_restriction = join_field.get_extra_restriction(
  1849
+                self.where_class, None, self.tables[pos + 1])
  1850
+            if extra_restriction:
  1851
+                self.where.add(extra_restriction, AND)
  1852
+        else:
  1853
+            # TODO: It might be possible to trim more joins from the start of the
  1854
+            # inner query if it happens to have a longer join chain containing the
  1855
+            # values in select_fields. Lets punt this one for now.
  1856
+            select_fields = [r[1] for r in join_field.related_fields]
  1857
+            select_alias = self.tables[pos]
1850 1858
         self.select = [SelectInfo((select_alias, f.column), f) for f in select_fields]
1851  
-        self.remove_inherited_models()
1852  
-        return pos
  1859
+        return trimmed_prefix, contains_louter
1853 1860
 
1854 1861
     def is_nullable(self, field):
1855 1862
         """
24  tests/generic_relations_regress/models.py
@@ -131,3 +131,27 @@ class Meta:
131 131
 
132 132
 class HasLinkThing(HasLinks):
133 133
     pass
  134
+
  135
+class A(models.Model):
  136
+    flag = models.NullBooleanField()
  137
+    content_type = models.ForeignKey(ContentType)
  138
+    object_id = models.PositiveIntegerField()
  139
+    content_object = generic.GenericForeignKey('content_type', 'object_id')
  140
+
  141
+class B(models.Model):
  142
+    a = generic.GenericRelation(A)
  143
+
  144
+    class Meta:
  145
+        ordering = ('id',)
  146
+
  147
+class C(models.Model):
  148
+    b = models.ForeignKey(B)
  149
+
  150
+    class Meta:
  151
+        ordering = ('id',)
  152
+
  153
+class D(models.Model):
  154
+    b = models.ForeignKey(B, null=True)
  155
+
  156
+    class Meta:
  157
+        ordering = ('id',)
58  tests/generic_relations_regress/tests.py
@@ -5,7 +5,7 @@
5 5
 from .models import (
6 6
     Address, Place, Restaurant, Link, CharLink, TextLink,
7 7
     Person, Contact, Note, Organization, OddRelation1, OddRelation2, Company,
8  
-    Developer, Team, Guild, Tag, Board, HasLinkThing)
  8
+    Developer, Team, Guild, Tag, Board, HasLinkThing, A, B, C, D)
9 9
 
10 10
 
11 11
 class GenericRelationTests(TestCase):
@@ -156,3 +156,59 @@ def test_ticket_20378(self):
156 156
         self.assertQuerysetEqual(
157 157
             HasLinkThing.objects.exclude(links=l1),
158 158
             [hs2], lambda x: x)
  159
+
  160
+    def test_ticket_20564(self):
  161
+        b1 = B.objects.create()
  162
+        b2 = B.objects.create()
  163
+        b3 = B.objects.create()
  164
+        c1 = C.objects.create(b=b1)
  165
+        c2 = C.objects.create(b=b2)
  166
+        c3 = C.objects.create(b=b3)
  167
+        A.objects.create(flag=None, content_object=b1)
  168
+        A.objects.create(flag=True, content_object=b2)
  169
+        self.assertQuerysetEqual(
  170
+            C.objects.filter(b__a__flag=None),
  171
+            [c1, c3], lambda x: x
  172
+        )
  173
+        self.assertQuerysetEqual(
  174
+            C.objects.exclude(b__a__flag=None),
  175
+            [c2], lambda x: x
  176
+        )
  177
+
  178
+    def test_ticket_20564_nullable_fk(self):
  179
+        b1 = B.objects.create()
  180
+        b2 = B.objects.create()
  181
+        b3 = B.objects.create()
  182
+        d1 = D.objects.create(b=b1)
  183
+        d2 = D.objects.create(b=b2)
  184
+        d3 = D.objects.create(b=b3)
  185
+        d4 = D.objects.create()
  186
+        A.objects.create(flag=None, content_object=b1)
  187
+        A.objects.create(flag=True, content_object=b1)
  188
+        A.objects.create(flag=True, content_object=b2)
  189
+        self.assertQuerysetEqual(
  190
+            D.objects.exclude(b__a__flag=None),
  191
+            [d2], lambda x: x
  192
+        )
  193
+        self.assertQuerysetEqual(
  194
+            D.objects.filter(b__a__flag=None),
  195
+            [d1, d3, d4], lambda x: x
  196
+        )
  197
+        self.assertQuerysetEqual(
  198
+            B.objects.filter(a__flag=None),
  199
+            [b1, b3], lambda x: x
  200
+        )
  201
+        self.assertQuerysetEqual(
  202
+            B.objects.exclude(a__flag=None),
  203
+            [b2], lambda x: x
  204
+        )
  205
+
  206
+    def test_extra_join_condition(self):
  207
+        # A crude check that content_type_id is taken in account in the
  208
+        # join/subquery condition.
  209
+        self.assertIn("content_type_id", str(B.objects.exclude(a__flag=None).query).lower())
  210
+        # No need for any joins - the join from inner query can be trimmed in
  211
+        # this case (but not in the above case as no a objects at all for given
  212
+        # B would then fail).
  213
+        self.assertNotIn(" join ", str(B.objects.exclude(a__flag=True).query).lower())
  214
+        self.assertIn("content_type_id", str(B.objects.exclude(a__flag=True).query).lower())

0 notes on commit 31fd64a

Please sign in to comment.
Something went wrong with that request. Please try again.