Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

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.
  • Loading branch information...
commit 8c72aa237918e31a525022f72b22cac75451af86 1 parent 2daf1ae
@akaariai akaariai authored
View
8 django/db/models/sql/compiler.py
@@ -455,6 +455,9 @@ def _setup_joins(self, pieces, opts, alias):
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 @@ def _setup_joins(self, pieces, opts, alias):
# 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):
View
2  tests/regressiontests/queries/models.py
@@ -264,7 +264,7 @@ def __unicode__(self):
return self.name
class RelatedObject(models.Model):
- single = models.ForeignKey(SingleObject)
+ single = models.ForeignKey(SingleObject, null=True)
class Meta:
ordering = ['single']
View
26 tests/regressiontests/queries/tests.py
@@ -19,7 +19,8 @@
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(),
['<Plaything: p1>']
)
+ 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,
+ ['<Plaything: p2>']
+ )
+
class DisjunctiveFilterTests(TestCase):
def setUp(self):
Please sign in to comment.
Something went wrong with that request. Please try again.