Ticket #7512: null_fk_ordering_r7722.patch

django/db/models/sql/query.py

@@ -1096 +1096 @@
         """
         joins = [alias]
         last = [0]
+        promoted = False
         for pos, name in enumerate(names):
             last.append(len(joins))
             if name == 'pk':
@@ -1119 +1120 @@
                 for alias in joins:
                     self.unref_alias(alias)
                 raise MultiJoin(pos + 1)
+
+            # Any time a null FK is encountered, promotion on the join 
+            # must occur, and must continue along all further joins.
+            # Necessary to do it this way since field could be either a
+            # Field subclass or a RelatedObject.
+            if getattr(field, 'null', None):
+                promoted = True
+
             if model:
                 # The field lives on a base class of the current model.
                 alias_list = []
@@ -1126 +1135 @@
                     lhs_col = opts.parents[int_model].column
                     opts = int_model._meta
                     alias = self.join((alias, opts.db_table, lhs_col,
-                            opts.pk.column), exclusions=joins)
+                            opts.pk.column), promote=promoted, exclusions=joins)
                     joins.append(alias)
             cached_data = opts._join_cache.get(name)
             orig_opts = opts
@@ -1151 +1160 @@
                                 target)
 
                     int_alias = self.join((alias, table1, from_col1, to_col1),
-                            dupe_multis, joins, nullable=True, reuse=can_reuse)
+                            dupe_multis, joins, promote=promoted, nullable=True, reuse=can_reuse)
                     alias = self.join((int_alias, table2, from_col2, to_col2),
-                            dupe_multis, joins, nullable=True, reuse=can_reuse)
+                            dupe_multis, joins, promote=promoted, nullable=True, reuse=can_reuse)
                     joins.extend([int_alias, alias])
                 elif field.rel:
                     # One-to-one or many-to-one field
@@ -1169 +1178 @@
                                 opts, target)
 
                     alias = self.join((alias, table, from_col, to_col),
-                            exclusions=joins, nullable=field.null)
+                            exclusions=joins, promote=promoted, nullable=field.null)
                     joins.append(alias)
                 else:
                     # Non-relation fields.
@@ -1178 +1187 @@
             else:
                 orig_field = field
                 field = field.field
+
+                # See comment above about null FKs.
+                if getattr(field, 'null', None):
+                    promoted = True
+
                 if m2m:
                     # Many-to-many field defined on the target model.
                     if cached_data:
@@ -1197 +1211 @@
                                 target)
 
                     int_alias = self.join((alias, table1, from_col1, to_col1),
-                            dupe_multis, joins, nullable=True, reuse=can_reuse)
+                            dupe_multis, joins, promote=promoted, nullable=True, reuse=can_reuse)
                     alias = self.join((int_alias, table2, from_col2, to_col2),
-                            dupe_multis, joins, nullable=True, reuse=can_reuse)
+                            dupe_multis, joins, promote=promoted, nullable=True, reuse=can_reuse)
                     joins.extend([int_alias, alias])
                 else:
                     # One-to-many field (ForeignKey defined on the target model)
@@ -1217 +1231 @@
                                 opts, target)
 
                     alias = self.join((alias, table, from_col, to_col),
-                            dupe_multis, joins, nullable=True, reuse=can_reuse)
+                            dupe_multis, joins, promote=promoted, nullable=True, reuse=can_reuse)
                     joins.append(alias)
 
         if pos != len(names) - 1:

new file tests/regressiontests/null_fk_ordering/models.py

@@ +1 @@
+"""
+Regression tests for proper working of ForeignKey(null=True). Tests these bugs:
+
+    * #7512: including a nullable foreign key reference in Meta ordering has unexpected results
+
+"""
+
+from django.db import models
+
+# The first two models represent a very simple null FK ordering case.
+class Author(models.Model):
+    name = models.CharField(max_length=150)
+
+class Article(models.Model):
+    title = models.CharField(max_length=150)
+    author = models.ForeignKey(Author, null=True)
+    
+    def __unicode__(self):
+        return u'Article titled: %s' % (self.title, )
+    
+    class Meta:
+        ordering = ['author__name', ]
+    
+
+# These following 4 models represent a far more complex ordering case.
+class SystemInfo(models.Model):
+    system_name = models.CharField(max_length=32)
+
+class Forum(models.Model):
+    system_info = models.ForeignKey(SystemInfo)
+    forum_name = models.CharField(max_length=32)
+
+class Post(models.Model):
+    forum = models.ForeignKey(Forum, null=True)
+    title = models.CharField(max_length=32)
+
+    def __unicode__(self):
+        return self.title
+
+class Comment(models.Model):
+    post = models.ForeignKey(Post, null=True)
+    comment_text = models.CharField(max_length=250)
+
+    class Meta:
+        ordering = ['post__forum__system_info__system_name', 'comment_text']
+
+    def __unicode__(self):
+        return self.comment_text
+
+__test__ = {'API_TESTS':"""
+
+class Author(models.Model):
+    name = models.CharField(max_length=150)
+
+class Article(models.Model):
+    title = models.CharField(max_length=150)
+    author = models.ForeignKey(Author, null=True)
+>>> author_1 = Author.objects.create(name='Tom Jones')
+>>> author_2 = Author.objects.create(name='Bob Smith')
+>>> article_1 = Article.objects.create(title='No author on this article')
+>>> article_2 = Article.objects.create(author=author_1, title='This article written by Tom Jones')
+>>> article_3 = Article.objects.create(author=author_2, title='This article written by Bob Smith')
+>>> Article.objects.all()
+[<Article: Article titled: No author on this article>, <Article: Article titled: This article written by Bob Smith>, <Article: Article titled: This article written by Tom Jones>]
+
+>>> s = SystemInfo.objects.create(system_name='System Info')
+>>> f = Forum.objects.create(system_info=s, forum_name='First forum')
+>>> p = Post.objects.create(forum=f, title='First Post')
+>>> c1 = Comment.objects.create(post=p, comment_text='My first comment')
+>>> c2 = Comment.objects.create(comment_text='My second comment')
+>>> s2 = SystemInfo.objects.create(system_name='More System Info')
+>>> f2 = Forum.objects.create(system_info=s2, forum_name='Second forum')
+>>> p2 = Post.objects.create(forum=f2, title='Second Post')
+>>> c3 = Comment.objects.create(comment_text='Another first comment')
+>>> c4 = Comment.objects.create(post=p2, comment_text='Another second comment')
+
+>>> Comment.objects.all()
+[<Comment: Another first comment>, <Comment: My second comment>, <Comment: Another second comment>, <Comment: My first comment>]
+
+"""}

tests/regressiontests/queries/models.py

@@ -230 +230 @@
 Checking that no join types are "left outer" joins.
 >>> query = Item.objects.filter(tags=t2).query
 >>> query.LOUTER not in [x[2] for x in query.alias_map.values()]
-True
+False
 
 >>> Item.objects.filter(Q(tags=t1)).order_by('name')
 [<Item: one>, <Item: two>]
@@ -479 +479 @@
 # ForeignKey) is legal, but the results might not make sense. That isn't
 # Django's problem. Garbage in, garbage out.
 >>> Item.objects.all().order_by('tags', 'id')
-[<Item: one>, <Item: two>, <Item: one>, <Item: two>, <Item: four>]
+[<Item: three>, <Item: one>, <Item: two>, <Item: one>, <Item: two>, <Item: four>]
 
 # If we replace the default ordering, Django adjusts the required tables
 # automatically. Item normally requires a join with Note to do the default