Ticket #16715: 16715_alternate.diff

File 16715_alternate.diff, 19.6 KB (added by Anssi Kääriäinen, 13 years ago)

  • django/db/models/sql/compiler.py 

    diff --git a/django/db/models/sql/compiler.py b/django/db/models/sql/compiler.py
index 05c19f3..b8e0e88 100644
    a b class SQLCompiler(object):  
    395395            self.query.ref_alias(alias)
    396396
    397397        # Must use left outer joins for nullable fields and their relations.
    398         self.query.promote_alias_chain(joins,
    399             self.query.alias_map[joins[0]][JOIN_TYPE] == self.query.LOUTER)
     398        self.query.promote_joins(joins)
    400399
    401400        # If we get to this point and the field is a relation to another model,
    402401        # append the default ordering for that model.
    … … class SQLCompiler(object):  
    577576                    alias_chain.append(alias)
    578577                    for (dupe_opts, dupe_col) in dupe_set:
    579578                        self.query.update_dupe_avoidance(dupe_opts, dupe_col, alias)
    580                 if self.query.alias_map[root_alias][JOIN_TYPE] == self.query.LOUTER:
    581                     self.query.promote_alias_chain(alias_chain, True)
     579                self.query.promote_joins(alias_chain, True)
    582580            else:
    583581                alias = root_alias
    584582
    … … class SQLCompiler(object):  
    595593            columns, aliases = self.get_default_columns(start_alias=alias,
    596594                    opts=f.rel.to._meta, as_pairs=True)
    597595            self.query.related_select_cols.extend(columns)
    598             if self.query.alias_map[alias][JOIN_TYPE] == self.query.LOUTER:
    599                 self.query.promote_alias_chain(aliases, True)
     596            self.query.promote_joins(aliases, True)
    600597            self.query.related_select_fields.extend(f.rel.to._meta.fields)
    601598            if restricted:
    602599                next = requested.get(f.name, {})
    … … class SQLCompiler(object):  
    669666                self.query.related_select_fields.extend(model._meta.fields)
    670667
    671668                next = requested.get(f.related_query_name(), {})
    672                 new_nullable = f.null or None
     669                # Use True here because we are looking at the _reverse_ side of
     670                # the relation, which is always nullable.
     671                new_nullable = True
    673672
    674673                self.fill_related_selections(model._meta, table, cur_depth+1,
    675674                    used, next, restricted, new_nullable)

  • django/db/models/sql/query.py 

    diff --git a/django/db/models/sql/query.py b/django/db/models/sql/query.py
index 110e317..2d68a9d 100644
    a b class Query(object):  
    479479            # Find aliases that are exclusive to rhs or lhs.
    480480            # These are promoted to outer joins.
    481481            outer_aliases = (l_tables | r_tables) - (l_tables & r_tables)
    482             for alias in outer_aliases:
    483                 self.promote_alias(alias, True)
     482            self.promote_joins(outer_aliases, True)
    484483
    485484        # Now relabel a copy of the rhs where-clause and add it to the current
    486485        # one.
    … … class Query(object):  
    661660        """ Decreases the reference count for this alias. """
    662661        self.alias_refcount[alias] -= 1
    663662
    664     def promote_alias(self, alias, unconditional=False):
    665         """
    666         Promotes the join type of an alias to an outer join if it's possible
    667         for the join to contain NULL values on the left. If 'unconditional' is
    668         False, the join is only promoted if it is nullable, otherwise it is
    669         always promoted.
    670 
    671         Returns True if the join was promoted.
    672         """
    673         if ((unconditional or self.alias_map[alias][NULLABLE]) and
    674                 self.alias_map[alias][JOIN_TYPE] != self.LOUTER):
    675             data = list(self.alias_map[alias])
    676             data[JOIN_TYPE] = self.LOUTER
    677             self.alias_map[alias] = tuple(data)
    678             return True
    679         return False
    680 
    681     def promote_alias_chain(self, chain, must_promote=False):
    682         """
    683         Walks along a chain of aliases, promoting the first nullable join and
    684         any joins following that. If 'must_promote' is True, all the aliases in
    685         the chain are promoted.
    686         """
    687         for alias in chain:
    688             if self.promote_alias(alias, must_promote):
    689                 must_promote = True
     663    def promote_joins(self, aliases, unconditional=False):
     664        """
     665        Promotes recursively the join type of given aliases and its childs to an outer
     666        join if it's possible for the join to contain NULL values on the left.
     667        If 'unconditional' is False, the join is only promoted if it is nullable or the
     668        parent join is an outer join. otherwise it is always promoted.
     669        """
     670        for alias in aliases:
     671            if alias not in ('auth_permission', 'django_content_type'):
     672                pass
     673                # import ipdb; ipdb.set_trace()
     674            parent_alias = self.alias_map[alias][LHS_ALIAS]
     675            parent_louter = parent_alias and self.alias_map[parent_alias][JOIN_TYPE] == self.LOUTER or False
     676            if ((unconditional or self.alias_map[alias][NULLABLE] or parent_louter) and
     677                    self.alias_map[alias][JOIN_TYPE] != self.LOUTER):
     678                data = list(self.alias_map[alias])
     679                data[JOIN_TYPE] = self.LOUTER
     680                self.alias_map[alias] = tuple(data)
    690681
    691682    def promote_unused_aliases(self, initial_refcounts, used_aliases):
    692683        """
    … … class Query(object):  
    696687        then and which ones haven't been used and promotes all of those
    697688        aliases, plus any children of theirs in the alias tree, to outer joins.
    698689        """
    699         # FIXME: There's some (a lot of!) overlap with the similar OR promotion
    700         # in add_filter(). It's not quite identical, but is very similar. So
    701         # pulling out the common bits is something for later.
    702         considered = {}
    703690        for alias in self.tables:
    704             if alias not in used_aliases:
    705                 continue
    706             if (alias not in initial_refcounts or
     691            if alias in used_aliases and (alias not in initial_refcounts or
    707692                    self.alias_refcount[alias] == initial_refcounts[alias]):
    708                 parent = self.alias_map[alias][LHS_ALIAS]
    709                 must_promote = considered.get(parent, False)
    710                 promoted = self.promote_alias(alias, must_promote)
    711                 considered[alias] = must_promote or promoted
     693                self.promote_joins([alias])
    712694
    713695    def change_aliases(self, change_map):
    714696        """
    … … class Query(object):  
    875857                        continue
    876858                    self.ref_alias(alias)
    877859                    if promote:
    878                         self.promote_alias(alias)
     860                        self.promote_joins([alias])
    879861                    return alias
    880862
    881863        # No reuse is possible, so we need a new alias.
    … … class Query(object):  
    984966            # If the aggregate references a model or field that requires a join,
    985967            # those joins must be LEFT OUTER - empty join rows must be returned
    986968            # in order for zeros to be returned for those aggregates.
    987             for column_alias in join_list:
    988                 self.promote_alias(column_alias, unconditional=True)
     969            self.promote_joins(join_list, unconditional=True)
    989970
    990971            col = (join_list[-1], col)
    991972        else:
    … … class Query(object):  
    10811062            # If the comparison is against NULL, we may need to use some left
    10821063            # outer joins when creating the join chain. This is only done when
    10831064            # needed, as it's less efficient at the database level.
    1084             self.promote_alias_chain(join_list)
     1065            self.promote_joins(join_list)
    10851066
    10861067        # Process the join list to see if we can remove any inner joins from
    10871068        # the far end (fewer tables in a query is better).
    … … class Query(object):  
    11021083                table = table_it.next()
    11031084                if join == table and self.alias_refcount[join] > 1:
    11041085                    continue
    1105                 join_promote = self.promote_alias(join)
     1086                join_promote = self.promote_joins([join])
    11061087                if table != join:
    1107                     table_promote = self.promote_alias(table)
     1088                    table_promote = self.promote_joins([table])
    11081089                break
    1109             self.promote_alias_chain(join_it, join_promote)
    1110             self.promote_alias_chain(table_it, table_promote)
    1111 
    11121090
    11131091        if having_clause or force_having:
    11141092            if (alias, col) not in self.group_by:
    … … class Query(object):  
    11201098                connector)
    11211099
    11221100        if negate:
    1123             self.promote_alias_chain(join_list)
     1101            self.promote_joins(join_list)
    11241102            if lookup_type != 'isnull':
    11251103                if len(join_list) > 1:
    11261104                    for alias in join_list:
    … … class Query(object):  
    15791557                        final_alias = join[LHS_ALIAS]
    15801558                        col = join[LHS_JOIN_COL]
    15811559                        joins = joins[:-1]
    1582                 self.promote_alias_chain(joins[1:])
     1560                self.promote_joins(joins[1:])
    15831561                self.select.append((final_alias, col))
    15841562                self.select_fields.append(field)
    15851563        except MultiJoin:

  • new file tests/regressiontests/nested_foreign_keys/models.py 

    diff --git a/tests/regressiontests/nested_foreign_keys/__init__.py b/tests/regressiontests/nested_foreign_keys/__init__.py
new file mode 100644
index 0000000..e69de29
diff --git a/tests/regressiontests/nested_foreign_keys/models.py b/tests/regressiontests/nested_foreign_keys/models.py
new file mode 100644
index 0000000..50d4479
    - +  
     1from django.db import models
     2
     3
     4class Person(models.Model):
     5    name = models.CharField(max_length=200)
     6
     7
     8class Movie(models.Model):
     9    title = models.CharField(max_length=200)
     10    director = models.ForeignKey(Person)
     11
     12
     13class Event(models.Model):
     14    pass
     15
     16
     17class Screening(Event):
     18    movie = models.ForeignKey(Movie)
     19
     20class ScreeningNullFK(Event):
     21    movie = models.ForeignKey(Movie, null=True)
     22
     23
     24class Package(models.Model):
     25    screening = models.ForeignKey(Screening, null=True)
     26
     27class PackageNullFK(models.Model):
     28    screening = models.ForeignKey(ScreeningNullFK, null=True)

  • new file tests/regressiontests/nested_foreign_keys/tests.py 

    diff --git a/tests/regressiontests/nested_foreign_keys/tests.py b/tests/regressiontests/nested_foreign_keys/tests.py
new file mode 100644
index 0000000..65e01f2
    - +  
     1from django.test import TestCase
     2
     3from models import Person, Movie, Event, Screening, ScreeningNullFK, Package, PackageNullFK
     4
     5
     6# These are tests for #16715. The basic scheme is always the same: 3 models with
     7# 2 relations. The first relation may be null, while the second is non-nullable.
     8# In some cases, Django would pick the wrong join type for the second relation,
     9# resulting in missing objects in the queryset.
     10#
     11#   Model A
     12#   | (Relation A/B : nullable)
     13#   Model B
     14#   | (Relation B/C : non-nullable)
     15#   Model C
     16#
     17# Because of the possibility of NULL rows resulting from the LEFT OUTER JOIN
     18# between Model A and Model B (i.e. instances of A without reference to B),
     19# the second join must also be LEFT OUTER JOIN, so that we do not ignore
     20# instances of A that do not reference B.
     21#
     22# Relation A/B can either be an explicit foreign key or an implicit reverse
     23# relation such as introduced by one-to-one relations (through multi-table
     24# inheritance).
     25class NestedForeignKeysTests(TestCase):
     26    def setUp(self):
     27        self.director = Person.objects.create(name=u'Terry Gilliam / Terry Jones')
     28        self.movie = Movie.objects.create(title=u'Monty Python and the Holy Grail', director=self.director)
     29
     30
     31    # This test failed in #16715 because in some cases INNER JOIN was selected
     32    # for the second foreign key relation instead of LEFT OUTER JOIN.
     33    def testInheritance(self):
     34        some_event = Event.objects.create()
     35        screening = Screening.objects.create(movie=self.movie)
     36
     37        self.assertEqual(len(Event.objects.all()), 2)
     38        self.assertEqual(len(Event.objects.select_related('screening')), 2)
     39        # This failed.
     40        self.assertEqual(len(Event.objects.select_related('screening__movie')), 2)
     41
     42        self.assertEqual(len(Event.objects.values()), 2)
     43        self.assertEqual(len(Event.objects.values('screening__pk')), 2)
     44        self.assertEqual(len(Event.objects.values('screening__movie__pk')), 2)
     45        self.assertEqual(len(Event.objects.values('screening__movie__title')), 2)
     46        # This failed.
     47        self.assertEqual(len(Event.objects.values('screening__movie__pk', 'screening__movie__title')), 2)
     48
     49        # Simple filter/exclude queries for good measure.
     50        self.assertEqual(Event.objects.filter(screening__movie=self.movie).count(), 1)
     51        self.assertEqual(Event.objects.exclude(screening__movie=self.movie).count(), 1)
     52
     53
     54    # These all work because the second foreign key in the chain has null=True.
     55    def testInheritanceNullFK(self):
     56        some_event = Event.objects.create()
     57        screening = ScreeningNullFK.objects.create(movie=None)
     58        screening_with_movie = ScreeningNullFK.objects.create(movie=self.movie)
     59
     60        self.assertEqual(len(Event.objects.all()), 3)
     61        self.assertEqual(len(Event.objects.select_related('screeningnullfk')), 3)
     62        self.assertEqual(len(Event.objects.select_related('screeningnullfk__movie')), 3)
     63
     64        self.assertEqual(len(Event.objects.values()), 3)
     65        self.assertEqual(len(Event.objects.values('screeningnullfk__pk')), 3)
     66        self.assertEqual(len(Event.objects.values('screeningnullfk__movie__pk')), 3)
     67        self.assertEqual(len(Event.objects.values('screeningnullfk__movie__title')), 3)
     68        self.assertEqual(len(Event.objects.values('screeningnullfk__movie__pk', 'screeningnullfk__movie__title')), 3)
     69
     70        self.assertEqual(Event.objects.filter(screeningnullfk__movie=self.movie).count(), 1)
     71        self.assertEqual(Event.objects.exclude(screeningnullfk__movie=self.movie).count(), 2)
     72
     73
     74    # This test failed in #16715 because in some cases INNER JOIN was selected
     75    # for the second foreign key relation instead of LEFT OUTER JOIN.
     76    def testExplicitForeignKey(self):
     77        package = Package.objects.create()
     78        screening = Screening.objects.create(movie=self.movie)
     79        package_with_screening = Package.objects.create(screening=screening)
     80
     81        self.assertEqual(len(Package.objects.all()), 2)
     82        self.assertEqual(len(Package.objects.select_related('screening')), 2)
     83        self.assertEqual(len(Package.objects.select_related('screening__movie')), 2)
     84
     85        self.assertEqual(len(Package.objects.values()), 2)
     86        self.assertEqual(len(Package.objects.values('screening__pk')), 2)
     87        self.assertEqual(len(Package.objects.values('screening__movie__pk')), 2)
     88        self.assertEqual(len(Package.objects.values('screening__movie__title')), 2)
     89        # This failed.
     90        self.assertEqual(len(Package.objects.values('screening__movie__pk', 'screening__movie__title')), 2)
     91
     92        self.assertEqual(Package.objects.filter(screening__movie=self.movie).count(), 1)
     93        self.assertEqual(Package.objects.exclude(screening__movie=self.movie).count(), 1)
     94
     95
     96    # These all work because the second foreign key in the chain has null=True.
     97    def testExplicitForeignKeyNullFK(self):
     98        package = PackageNullFK.objects.create()
     99        screening = ScreeningNullFK.objects.create(movie=None)
     100        screening_with_movie = ScreeningNullFK.objects.create(movie=self.movie)
     101        package_with_screening = PackageNullFK.objects.create(screening=screening)
     102        package_with_screening_with_movie = PackageNullFK.objects.create(screening=screening_with_movie)
     103
     104        self.assertEqual(len(PackageNullFK.objects.all()), 3)
     105        self.assertEqual(len(PackageNullFK.objects.select_related('screening')), 3)
     106        self.assertEqual(len(PackageNullFK.objects.select_related('screening__movie')), 3)
     107
     108        self.assertEqual(len(PackageNullFK.objects.values()), 3)
     109        self.assertEqual(len(PackageNullFK.objects.values('screening__pk')), 3)
     110        self.assertEqual(len(PackageNullFK.objects.values('screening__movie__pk')), 3)
     111        self.assertEqual(len(PackageNullFK.objects.values('screening__movie__title')), 3)
     112        self.assertEqual(len(PackageNullFK.objects.values('screening__movie__pk', 'screening__movie__title')), 3)
     113
     114        self.assertEqual(PackageNullFK.objects.filter(screening__movie=self.movie).count(), 1)
     115        self.assertEqual(PackageNullFK.objects.exclude(screening__movie=self.movie).count(), 2)
     116
     117
     118# Some additional tests for #16715. The only difference is the depth of the
     119# nesting as we now use 4 models instead of 3 (and thus 3 relations). This
     120# checks if promotion of join types works for deeper nesting too.
     121class DeeplyNestedForeignKeysTests(TestCase):
     122    def setUp(self):
     123        self.director = Person.objects.create(name=u'Terry Gilliam / Terry Jones')
     124        self.movie = Movie.objects.create(title=u'Monty Python and the Holy Grail', director=self.director)
     125
     126
     127    def testInheritance(self):
     128        some_event = Event.objects.create()
     129        screening = Screening.objects.create(movie=self.movie)
     130
     131        self.assertEqual(len(Event.objects.all()), 2)
     132        self.assertEqual(len(Event.objects.select_related('screening__movie__director')), 2)
     133
     134        self.assertEqual(len(Event.objects.values()), 2)
     135        self.assertEqual(len(Event.objects.values('screening__movie__director__pk')), 2)
     136        self.assertEqual(len(Event.objects.values('screening__movie__director__name')), 2)
     137        self.assertEqual(len(Event.objects.values('screening__movie__director__pk', 'screening__movie__director__name')), 2)
     138        self.assertEqual(len(Event.objects.values('screening__movie__pk', 'screening__movie__director__pk')), 2)
     139        self.assertEqual(len(Event.objects.values('screening__movie__pk', 'screening__movie__director__name')), 2)
     140        self.assertEqual(len(Event.objects.values('screening__movie__title', 'screening__movie__director__pk')), 2)
     141        self.assertEqual(len(Event.objects.values('screening__movie__title', 'screening__movie__director__name')), 2)
     142
     143        self.assertEqual(Event.objects.filter(screening__movie__director=self.director).count(), 1)
     144        self.assertEqual(Event.objects.exclude(screening__movie__director=self.director).count(), 1)
     145
     146
     147    def testExplicitForeignKey(self):
     148        package = Package.objects.create()
     149        screening = Screening.objects.create(movie=self.movie)
     150        package_with_screening = Package.objects.create(screening=screening)
     151
     152        self.assertEqual(len(Package.objects.all()), 2)
     153        self.assertEqual(len(Package.objects.select_related('screening__movie__director')), 2)
     154
     155        self.assertEqual(len(Package.objects.values()), 2)
     156        self.assertEqual(len(Package.objects.values('screening__movie__director__pk')), 2)
     157        self.assertEqual(len(Package.objects.values('screening__movie__director__name')), 2)
     158        self.assertEqual(len(Package.objects.values('screening__movie__director__pk', 'screening__movie__director__name')), 2)
     159        self.assertEqual(len(Package.objects.values('screening__movie__pk', 'screening__movie__director__pk')), 2)
     160        self.assertEqual(len(Package.objects.values('screening__movie__pk', 'screening__movie__director__name')), 2)
     161        self.assertEqual(len(Package.objects.values('screening__movie__title', 'screening__movie__director__pk')), 2)
     162        self.assertEqual(len(Package.objects.values('screening__movie__title', 'screening__movie__director__name')), 2)
     163
     164        self.assertEqual(Package.objects.filter(screening__movie__director=self.director).count(), 1)
     165        self.assertEqual(Package.objects.exclude(screening__movie__director=self.director).count(), 1)
Back to Top