Skip to content
This repository

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse code

Improved table join handling for comparisons against NULL.

This fixes a broad class of bugs involving filters that look for missing
related models and fields. Most of them don't seem to have been reported
(the added tests cover the root cause). The exception is that this has
also fixed #9868.

git-svn-id: http://code.djangoproject.com/svn/django/trunk@9979 bcc190cf-cafb-0310-a4f2-bffc1f526a37
  • Loading branch information...
commit 923f78f5046bd35f5063aa06c1cfeff013317636 1 parent 68f81c8
Malcolm Tredinnick authored March 06, 2009
2  django/db/models/sql/expressions.py
@@ -42,7 +42,7 @@ def prepare_leaf(self, node, query, allow_joins):
42 42
                 field, source, opts, join_list, last, _ = query.setup_joins(
43 43
                     field_list, query.get_meta(),
44 44
                     query.get_initial_alias(), False)
45  
-                _, _, col, _, join_list = query.trim_joins(source, join_list, last, False)
  45
+                col, _, join_list = query.trim_joins(source, join_list, last, False)
46 46
 
47 47
                 self.cols[node] = (join_list[-1], col)
48 48
             except FieldDoesNotExist:
53  django/db/models/sql/query.py
@@ -1287,7 +1287,7 @@ def add_aggregate(self, aggregate, model, alias, is_summary):
1287 1287
                 field_list, opts, self.get_initial_alias(), False)
1288 1288
 
1289 1289
             # Process the join chain to see if it can be trimmed
1290  
-            _, _, col, _, join_list = self.trim_joins(source, join_list, last, False)
  1290
+            col, _, join_list = self.trim_joins(source, join_list, last, False)
1291 1291
 
1292 1292
             # If the aggregate references a model or field that requires a join,
1293 1293
             # those joins must be LEFT OUTER - empty join rows must be returned
@@ -1392,16 +1392,16 @@ def add_filter(self, filter_expr, connector=AND, negate=False, trim=False,
1392 1392
                     can_reuse)
1393 1393
             return
1394 1394
 
1395  
-        # Process the join chain to see if it can be trimmed
1396  
-        final, penultimate, col, alias, join_list = self.trim_joins(target, join_list, last, trim)
1397  
-
1398 1395
         if (lookup_type == 'isnull' and value is True and not negate and
1399  
-                final > 1):
1400  
-            # If the comparison is against NULL, we need to use a left outer
1401  
-            # join when connecting to the previous model. We make that
1402  
-            # adjustment here. We don't do this unless needed as it's less
1403  
-            # efficient at the database level.
1404  
-            self.promote_alias(join_list[penultimate])
  1396
+                len(join_list) > 1):
  1397
+            # If the comparison is against NULL, we may need to use some left
  1398
+            # outer joins when creating the join chain. This is only done when
  1399
+            # needed, as it's less efficient at the database level.
  1400
+            self.promote_alias_chain(join_list)
  1401
+
  1402
+        # Process the join list to see if we can remove any inner joins from
  1403
+        # the far end (fewer tables in a query is better).
  1404
+        col, alias, join_list = self.trim_joins(target, join_list, last, trim)
1405 1405
 
1406 1406
         if connector == OR:
1407 1407
             # Some joins may need to be promoted when adding a new filter to a
@@ -1436,7 +1436,7 @@ def add_filter(self, filter_expr, connector=AND, negate=False, trim=False,
1436 1436
         if negate:
1437 1437
             self.promote_alias_chain(join_list)
1438 1438
             if lookup_type != 'isnull':
1439  
-                if final > 1:
  1439
+                if len(join_list) > 1:
1440 1440
                     for alias in join_list:
1441 1441
                         if self.alias_map[alias][JOIN_TYPE] == self.LOUTER:
1442 1442
                             j_col = self.alias_map[alias][RHS_JOIN_COL]
@@ -1699,13 +1699,28 @@ def setup_joins(self, names, opts, alias, dupe_multis, allow_many=True,
1699 1699
         return field, target, opts, joins, last, extra_filters
1700 1700
 
1701 1701
     def trim_joins(self, target, join_list, last, trim):
1702  
-        """An optimization: if the final join is against the same column as
1703  
-        we are comparing against, we can go back one step in a join
1704  
-        chain and compare against the LHS of the join instead (and then
1705  
-        repeat the optimization). The result, potentially, involves less
1706  
-        table joins.
  1702
+        """
  1703
+        Sometimes joins at the end of a multi-table sequence can be trimmed. If
  1704
+        the final join is against the same column as we are comparing against,
  1705
+        and is an inner join, we can go back one step in a join chain and
  1706
+        compare against the LHS of the join instead (and then repeat the
  1707
+        optimization). The result, potentially, involves less table joins.
  1708
+
  1709
+        The 'target' parameter is the final field being joined to, 'join_list'
  1710
+        is the full list of join aliases.
  1711
+
  1712
+        The 'last' list contains offsets into 'join_list', corresponding to
  1713
+        each component of the filter.  Many-to-many relations, for example, add
  1714
+        two tables to the join list and we want to deal with both tables the
  1715
+        same way, so 'last' has an entry for the first of the two tables and
  1716
+        then the table immediately after the second table, in that case.
  1717
+
  1718
+        The 'trim' parameter forces the final piece of the join list to be
  1719
+        trimmed before anything. See the documentation of add_filter() for
  1720
+        details about this.
1707 1721
 
1708  
-        Returns a tuple
  1722
+        Returns the final active column and table alias and the new active
  1723
+        join_list.
1709 1724
         """
1710 1725
         final = len(join_list)
1711 1726
         penultimate = last.pop()
@@ -1724,7 +1739,7 @@ def trim_joins(self, target, join_list, last, trim):
1724 1739
         alias = join_list[-1]
1725 1740
         while final > 1:
1726 1741
             join = self.alias_map[alias]
1727  
-            if col != join[RHS_JOIN_COL]:
  1742
+            if col != join[RHS_JOIN_COL] or join[JOIN_TYPE] != self.INNER:
1728 1743
                 break
1729 1744
             self.unref_alias(alias)
1730 1745
             alias = join[LHS_ALIAS]
@@ -1733,7 +1748,7 @@ def trim_joins(self, target, join_list, last, trim):
1733 1748
             final -= 1
1734 1749
             if final == penultimate:
1735 1750
                 penultimate = last.pop()
1736  
-        return final, penultimate, col, alias, join_list
  1751
+        return col, alias, join_list
1737 1752
 
1738 1753
     def update_dupe_avoidance(self, opts, col, alias):
1739 1754
         """
23  tests/regressiontests/null_queries/models.py
@@ -13,6 +13,17 @@ class Choice(models.Model):
13 13
     def __unicode__(self):
14 14
         return u"Choice: %s in poll %s" % (self.choice, self.poll)
15 15
 
  16
+# A set of models with an inner one pointing to two outer ones.
  17
+class OuterA(models.Model):
  18
+    pass
  19
+
  20
+class OuterB(models.Model):
  21
+    data = models.CharField(max_length=10)
  22
+
  23
+class Inner(models.Model):
  24
+    first = models.ForeignKey(OuterA)
  25
+    second = models.ForeignKey(OuterB, null=True)
  26
+
16 27
 __test__ = {'API_TESTS':"""
17 28
 # Regression test for the use of None as a query value. None is interpreted as
18 29
 # an SQL NULL, but only in __exact queries.
@@ -56,4 +67,16 @@ def __unicode__(self):
56 67
 >>> p2.choice_set.all()
57 68
 []
58 69
 
  70
+# Querying across reverse relations and then another relation should insert
  71
+# outer joins correctly so as not to exclude results.
  72
+>>> obj = OuterA.objects.create()
  73
+>>> OuterA.objects.filter(inner__second=None)
  74
+[<OuterA: OuterA object>]
  75
+>>> OuterA.objects.filter(inner__second__data=None)
  76
+[<OuterA: OuterA object>]
  77
+>>> _ = Inner.objects.create(first=obj)
  78
+>>> Inner.objects.filter(first__inner__second=None)
  79
+[<Inner: Inner object>]
  80
+
  81
+
59 82
 """}
22  tests/regressiontests/one_to_one_regress/models.py
@@ -33,6 +33,15 @@ class Favorites(models.Model):
33 33
     def __unicode__(self):
34 34
         return u"Favorites for %s" % self.name
35 35
 
  36
+class Target(models.Model):
  37
+    pass
  38
+
  39
+class Pointer(models.Model):
  40
+    other = models.OneToOneField(Target, primary_key=True)
  41
+
  42
+class Pointer2(models.Model):
  43
+    other = models.OneToOneField(Target)
  44
+
36 45
 __test__ = {'API_TESTS':"""
37 46
 # Regression test for #1064 and #1506: Check that we create models via the m2m
38 47
 # relation if the remote model has a OneToOneField.
@@ -119,4 +128,17 @@ def __unicode__(self):
119 128
 >>> r.place == p
120 129
 True
121 130
 
  131
+# Regression test for #9968: filtering reverse one-to-one relations with
  132
+# primary_key=True was misbehaving. We test both (primary_key=True & False)
  133
+# cases here to prevent any reappearance of the problem.
  134
+>>> _ = Target.objects.create()
  135
+>>> Target.objects.filter(pointer=None)
  136
+[<Target: Target object>]
  137
+>>> Target.objects.exclude(pointer=None)
  138
+[]
  139
+>>> Target.objects.filter(pointer2=None)
  140
+[<Target: Target object>]
  141
+>>> Target.objects.exclude(pointer2=None)
  142
+[]
  143
+
122 144
 """}

0 notes on commit 923f78f

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