Ticket #16715: 16715_alternate-redux-r16928.diff

File 16715_alternate-redux-r16928.diff, 20.2 KB (added by Sebastian Goll, 13 years ago)

  • tests/regressiontests/nested_foreign_keys/tests.py

     
     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)

  • tests/regressiontests/nested_foreign_keys/models.py

     
     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)

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

     
    397397            self.query.ref_alias(alias)
    398398
    399399        # Must use left outer joins for nullable fields and their relations.
    400         self.query.promote_alias_chain(joins,
    401             self.query.alias_map[joins[0]][JOIN_TYPE] == self.query.LOUTER)
     400        self.query.promote_joins(joins)
    402401
    403402        # If we get to this point and the field is a relation to another model,
    404403        # append the default ordering for that model.
    … …  
    579578                    alias_chain.append(alias)
    580579                    for (dupe_opts, dupe_col) in dupe_set:
    581580                        self.query.update_dupe_avoidance(dupe_opts, dupe_col, alias)
    582                 if self.query.alias_map[root_alias][JOIN_TYPE] == self.query.LOUTER:
    583                     self.query.promote_alias_chain(alias_chain, True)
     581                self.query.promote_joins(alias_chain)
    584582            else:
    585583                alias = root_alias
    586584
    … …  
    597595            columns, aliases = self.get_default_columns(start_alias=alias,
    598596                    opts=f.rel.to._meta, as_pairs=True)
    599597            self.query.related_select_cols.extend(columns)
    600             if self.query.alias_map[alias][JOIN_TYPE] == self.query.LOUTER:
    601                 self.query.promote_alias_chain(aliases, True)
     598            self.query.promote_joins(aliases)
    602599            self.query.related_select_fields.extend(f.rel.to._meta.fields)
    603600            if restricted:
    604601                next = requested.get(f.name, {})
    … …  
    671668                self.query.related_select_fields.extend(model._meta.fields)
    672669
    673670                next = requested.get(f.related_query_name(), {})
    674                 new_nullable = f.null or None
     671                # Use True here because we are looking at the _reverse_ side of
     672                # the relation, which is always nullable.
     673                new_nullable = True
    675674
    676675                self.fill_related_selections(model._meta, table, cur_depth+1,
    677676                    used, next, restricted, new_nullable)

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

     
    497497                # Again, some of the tables won't have aliases due to
    498498                # the trimming of unnecessary tables.
    499499                if self.alias_refcount.get(alias) or rhs.alias_refcount.get(alias):
    500                     self.promote_alias(alias, True)
     500                    self.promote_joins([alias], True)
    501501
    502502        # Now relabel a copy of the rhs where-clause and add it to the current
    503503        # one.
    … …  
    677677        """ Decreases the reference count for this alias. """
    678678        self.alias_refcount[alias] -= 1
    679679
    680     def promote_alias(self, alias, unconditional=False):
     680    def promote_joins(self, aliases, unconditional=False):
    681681        """
    682         Promotes the join type of an alias to an outer join if it's possible
    683         for the join to contain NULL values on the left. If 'unconditional' is
    684         False, the join is only promoted if it is nullable, otherwise it is
    685         always promoted.
    686 
    687         Returns True if the join was promoted by this call.
     682        Promotes recursively the join type of given aliases and its children to
     683        an outer join if it's possible for the join to contain NULL values on
     684        the left.  If 'unconditional' is False, the join is only promoted if it
     685        is nullable or the parent join is an outer join, otherwise it is always
     686        promoted.
    688687        """
    689         if ((unconditional or self.alias_map[alias][NULLABLE]) and
    690                 self.alias_map[alias][JOIN_TYPE] != self.LOUTER):
    691             data = list(self.alias_map[alias])
    692             data[JOIN_TYPE] = self.LOUTER
    693             self.alias_map[alias] = tuple(data)
    694             return True
    695         return False
     688        aliases = list(aliases)
     689        while aliases:
     690            alias = aliases.pop(0)
     691            parent_alias = self.alias_map[alias][LHS_ALIAS]
     692            parent_louter = parent_alias and self.alias_map[parent_alias][JOIN_TYPE] == self.LOUTER or False
     693            if ((unconditional or self.alias_map[alias][NULLABLE] or parent_louter) and
     694                    self.alias_map[alias][JOIN_TYPE] != self.LOUTER):
     695                data = list(self.alias_map[alias])
     696                data[JOIN_TYPE] = self.LOUTER
     697                self.alias_map[alias] = tuple(data)
     698                # Join type of 'alias' changed, so re-examine all aliases that
     699                # refer to this one (and, in turn, their children).
     700                aliases.extend(child for child in self.alias_map.iterkeys()
     701                    if self.alias_map[child][LHS_ALIAS] == alias and child not in aliases)
    696702
    697     def promote_alias_chain(self, chain, must_promote=False):
    698         """
    699         Walks along a chain of aliases, promoting the first nullable join and
    700         any joins following that. If 'must_promote' is True, all the aliases in
    701         the chain are promoted.
    702         """
    703         for alias in chain:
    704             if self.promote_alias(alias, must_promote):
    705                 must_promote = True
    706 
    707703    def promote_unused_aliases(self, initial_refcounts, used_aliases):
    708704        """
    709705        Given a "before" copy of the alias_refcounts dictionary (as
    … …  
    712708        then and which ones haven't been used and promotes all of those
    713709        aliases, plus any children of theirs in the alias tree, to outer joins.
    714710        """
    715         # FIXME: There's some (a lot of!) overlap with the similar OR promotion
    716         # in add_filter(). It's not quite identical, but is very similar. So
    717         # pulling out the common bits is something for later.
    718         considered = {}
    719711        for alias in self.tables:
    720             if alias not in used_aliases:
    721                 continue
    722             if (alias not in initial_refcounts or
     712            if alias in used_aliases and (alias not in initial_refcounts or
    723713                    self.alias_refcount[alias] == initial_refcounts[alias]):
    724                 parent = self.alias_map[alias][LHS_ALIAS]
    725                 must_promote = considered.get(parent, False)
    726                 promoted = self.promote_alias(alias, must_promote)
    727                 considered[alias] = must_promote or promoted
     714                self.promote_joins([alias])
    728715
    729716    def change_aliases(self, change_map):
    730717        """
    … …  
    891878                        continue
    892879                    self.ref_alias(alias)
    893880                    if promote:
    894                         self.promote_alias(alias)
     881                        self.promote_joins([alias])
    895882                    return alias
    896883
    897884        # No reuse is possible, so we need a new alias.
    … …  
    1000987            # If the aggregate references a model or field that requires a join,
    1001988            # those joins must be LEFT OUTER - empty join rows must be returned
    1002989            # in order for zeros to be returned for those aggregates.
    1003             for column_alias in join_list:
    1004                 self.promote_alias(column_alias, unconditional=True)
     990            self.promote_joins(join_list, True)
    1005991
    1006992            col = (join_list[-1], col)
    1007993        else:
    … …  
    11001086            # If the comparison is against NULL, we may need to use some left
    11011087            # outer joins when creating the join chain. This is only done when
    11021088            # needed, as it's less efficient at the database level.
    1103             self.promote_alias_chain(join_list)
     1089            self.promote_joins(join_list)
    11041090            join_promote = True
    11051091
    11061092        # Process the join list to see if we can remove any inner joins from
    … …  
    11311117                    # This means that we are dealing with two different query
    11321118                    # subtrees, so we don't need to do any join promotion.
    11331119                    continue
    1134                 join_promote = join_promote or self.promote_alias(join, unconditional)
     1120                join_promote = join_promote or self.promote_joins([join], unconditional)
    11351121                if table != join:
    1136                     table_promote = self.promote_alias(table)
     1122                    table_promote = self.promote_joins([table])
    11371123                # We only get here if we have found a table that exists
    11381124                # in the join list, but isn't on the original tables list.
    11391125                # This means we've reached the point where we only have
    11401126                # new tables, so we can break out of this promotion loop.
    11411127                break
    1142             self.promote_alias_chain(join_it, join_promote)
    1143             self.promote_alias_chain(table_it, table_promote or join_promote)
     1128            self.promote_joins(join_it, join_promote)
     1129            self.promote_joins(table_it, table_promote or join_promote)
    11441130
    11451131        if having_clause or force_having:
    11461132            if (alias, col) not in self.group_by:
    … …  
    11521138                connector)
    11531139
    11541140        if negate:
    1155             self.promote_alias_chain(join_list)
     1141            self.promote_joins(join_list)
    11561142            if lookup_type != 'isnull':
    11571143                if len(join_list) > 1:
    11581144                    for alias in join_list:
    … …  
    16171603                        final_alias = join[LHS_ALIAS]
    16181604                        col = join[LHS_JOIN_COL]
    16191605                        joins = joins[:-1]
    1620                 self.promote_alias_chain(joins[1:])
     1606                self.promote_joins(joins[1:])
    16211607                self.select.append((final_alias, col))
    16221608                self.select_fields.append(field)
    16231609        except MultiJoin:
Back to Top