From 8c72aa237918e31a525022f72b22cac75451af86 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Anssi=20K=C3=A4=C3=A4ri=C3=A4inen?= Date: Wed, 23 May 2012 23:25:23 +0300 Subject: [PATCH] Fixed qs.order_by() join promotion for already existing joins When order_by causes new joins to be added to the query, the joins must be LEFT OUTER joins for nullable relations, otherwise the order_by could cause the results to be altered. This commit fixes the logic to only promote new joins, previously all joins in the order_by lookup path were promoted. Thanks to Bruno Desthuilliers for spotting this corner case. --- django/db/models/sql/compiler.py | 8 ++++++-- tests/regressiontests/queries/models.py | 2 +- tests/regressiontests/queries/tests.py | 26 +++++++++++++++++++++++-- 3 files changed, 31 insertions(+), 5 deletions(-) diff --git a/django/db/models/sql/compiler.py b/django/db/models/sql/compiler.py index 36d45b7cb61..5801b2f4280 100644 --- a/django/db/models/sql/compiler.py +++ b/django/db/models/sql/compiler.py @@ -455,6 +455,9 @@ class SQLCompiler(object): alias = self.query.get_initial_alias() field, target, opts, joins, _, _ = self.query.setup_joins(pieces, opts, alias, False) + # We will later on need to promote those joins that were added to the + # query afresh above. + joins_to_promote = [j for j in joins if self.query.alias_refcount[j] < 2] alias = joins[-1] col = target.column if not field.rel: @@ -466,8 +469,9 @@ class SQLCompiler(object): # Must use left outer joins for nullable fields and their relations. # Ordering or distinct must not affect the returned set, and INNER # JOINS for nullable fields could do this. - self.query.promote_alias_chain(joins, - self.query.alias_map[joins[0]].join_type == self.query.LOUTER) + if joins_to_promote: + self.query.promote_alias_chain(joins_to_promote, + self.query.alias_map[joins_to_promote[0]].join_type == self.query.LOUTER) return field, col, alias, joins, opts def _final_join_removal(self, col, alias): diff --git a/tests/regressiontests/queries/models.py b/tests/regressiontests/queries/models.py index 07aa6db67b0..e4ce9d7b60c 100644 --- a/tests/regressiontests/queries/models.py +++ b/tests/regressiontests/queries/models.py @@ -264,7 +264,7 @@ class SingleObject(models.Model): return self.name class RelatedObject(models.Model): - single = models.ForeignKey(SingleObject) + single = models.ForeignKey(SingleObject, null=True) class Meta: ordering = ['single'] diff --git a/tests/regressiontests/queries/tests.py b/tests/regressiontests/queries/tests.py index 14a693f2c5f..28aa63328ea 100644 --- a/tests/regressiontests/queries/tests.py +++ b/tests/regressiontests/queries/tests.py @@ -19,7 +19,8 @@ from .models import (Annotation, Article, Author, Celebrity, Child, Cover, ManagedModel, Member, NamedCategory, Note, Number, Plaything, PointerA, Ranking, Related, Report, ReservedName, Tag, TvChef, Valid, X, Food, Eaten, Node, ObjectA, ObjectB, ObjectC, CategoryItem, SimpleCategory, - SpecialCategory, OneToOneCategory, NullableName, ProxyCategory) + SpecialCategory, OneToOneCategory, NullableName, ProxyCategory, + SingleObject, RelatedObject) class BaseQuerysetTest(TestCase): @@ -1321,12 +1322,33 @@ class NullableRelOrderingTests(TestCase): def test_ticket10028(self): # Ordering by model related to nullable relations(!) should use outer # joins, so that all results are included. - _ = Plaything.objects.create(name="p1") + Plaything.objects.create(name="p1") self.assertQuerysetEqual( Plaything.objects.all(), [''] ) + def test_join_already_in_query(self): + # Ordering by model related to nullable relations should not change + # the join type of already existing joins. + Plaything.objects.create(name="p1") + s = SingleObject.objects.create(name='s') + r = RelatedObject.objects.create(single=s) + Plaything.objects.create(name="p2", others=r) + qs = Plaything.objects.all().filter(others__isnull=False).order_by('pk') + self.assertTrue('INNER' in str(qs.query)) + qs = qs.order_by('others__single__name') + # The ordering by others__single__pk will add one new join (to single) + # and that join must be LEFT join. The already existing join to related + # objects must be kept INNER. So, we have both a INNER and a LEFT join + # in the query. + self.assertTrue('LEFT' in str(qs.query)) + self.assertTrue('INNER' in str(qs.query)) + self.assertQuerysetEqual( + qs, + [''] + ) + class DisjunctiveFilterTests(TestCase): def setUp(self):