Skip to content
This repository

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse code

Fixed #14056 -- Made sure LEFT JOIN aren't trimmed in ORDER BY

If LEFT JOINs are required for correct results, then trimming the join
can lead to incorrect results. Consider case:

TBL A: ID | TBL B: ID  A_ID
       1           1   1
       2
Now A.order_by('b__a') did use a join to B, and B's a_id column. This
was seen to contain the same value as A's id, and so the join was
trimmed. But this wasn't correct as the join is LEFT JOIN, and for row
A.id = 2 the B.a_id column is NULL.
  • Loading branch information...
commit 905409855c6b69f613190fcc2d8bd8bf5e1580b5 1 parent b53ed35
Anssi Kääriäinen authored August 20, 2013
46  django/db/models/sql/compiler.py
@@ -319,10 +319,10 @@ def get_distinct(self):
319 319
 
320 320
         for name in self.query.distinct_fields:
321 321
             parts = name.split(LOOKUP_SEP)
322  
-            field, cols, alias, _, _ = self._setup_joins(parts, opts, None)
323  
-            cols, alias = self._final_join_removal(cols, alias)
324  
-            for col in cols:
325  
-                result.append("%s.%s" % (qn(alias), qn2(col)))
  322
+            _, targets, alias, joins, path, _ = self._setup_joins(parts, opts, None)
  323
+            targets, alias, _ = self.query.trim_joins(targets, joins, path)
  324
+            for target in targets:
  325
+                result.append("%s.%s" % (qn(alias), qn2(target.column)))
326 326
         return result
327 327
 
328 328
     def get_ordering(self):
@@ -421,7 +421,7 @@ def get_ordering(self):
421 421
         return result, params, group_by
422 422
 
423 423
     def find_ordering_name(self, name, opts, alias=None, default_order='ASC',
424  
-            already_seen=None):
  424
+                           already_seen=None):
425 425
         """
426 426
         Returns the table alias (the name might be ambiguous, the alias will
427 427
         not be) and column name for ordering by the given 'name' parameter.
@@ -429,11 +429,11 @@ def find_ordering_name(self, name, opts, alias=None, default_order='ASC',
429 429
         """
430 430
         name, order = get_order_dir(name, default_order)
431 431
         pieces = name.split(LOOKUP_SEP)
432  
-        field, cols, alias, joins, opts = self._setup_joins(pieces, opts, alias)
  432
+        field, targets, alias, joins, path, opts = self._setup_joins(pieces, opts, alias)
433 433
 
434 434
         # If we get to this point and the field is a relation to another model,
435 435
         # append the default ordering for that model.
436  
-        if field.rel and len(joins) > 1 and opts.ordering:
  436
+        if field.rel and path and opts.ordering:
437 437
             # Firstly, avoid infinite loops.
438 438
             if not already_seen:
439 439
                 already_seen = set()
@@ -445,10 +445,10 @@ def find_ordering_name(self, name, opts, alias=None, default_order='ASC',
445 445
             results = []
446 446
             for item in opts.ordering:
447 447
                 results.extend(self.find_ordering_name(item, opts, alias,
448  
-                        order, already_seen))
  448
+                                                       order, already_seen))
449 449
             return results
450  
-        cols, alias = self._final_join_removal(cols, alias)
451  
-        return [(alias, cols, order)]
  450
+        targets, alias, _ = self.query.trim_joins(targets, joins, path)
  451
+        return [(alias, [t.column for t in targets], order)]
452 452
 
453 453
     def _setup_joins(self, pieces, opts, alias):
454 454
         """
@@ -461,13 +461,12 @@ def _setup_joins(self, pieces, opts, alias):
461 461
         """
462 462
         if not alias:
463 463
             alias = self.query.get_initial_alias()
464  
-        field, targets, opts, joins, _ = self.query.setup_joins(
  464
+        field, targets, opts, joins, path = self.query.setup_joins(
465 465
             pieces, opts, alias)
466 466
         # We will later on need to promote those joins that were added to the
467 467
         # query afresh above.
468 468
         joins_to_promote = [j for j in joins if self.query.alias_refcount[j] < 2]
469 469
         alias = joins[-1]
470  
-        cols = [target.column for target in targets]
471 470
         if not field.rel:
472 471
             # To avoid inadvertent trimming of a necessary alias, use the
473 472
             # refcount to show that we are referencing a non-relation field on
@@ -478,28 +477,7 @@ def _setup_joins(self, pieces, opts, alias):
478 477
         # Ordering or distinct must not affect the returned set, and INNER
479 478
         # JOINS for nullable fields could do this.
480 479
         self.query.promote_joins(joins_to_promote)
481  
-        return field, cols, alias, joins, opts
482  
-
483  
-    def _final_join_removal(self, cols, alias):
484  
-        """
485  
-        A helper method for get_distinct and get_ordering. This method will
486  
-        trim extra not-needed joins from the tail of the join chain.
487  
-
488  
-        This is very similar to what is done in trim_joins, but we will
489  
-        trim LEFT JOINS here. It would be a good idea to consolidate this
490  
-        method and query.trim_joins().
491  
-        """
492  
-        if alias:
493  
-            while 1:
494  
-                join = self.query.alias_map[alias]
495  
-                lhs_cols, rhs_cols = zip(*[(lhs_col, rhs_col) for lhs_col, rhs_col in join.join_cols])
496  
-                if set(cols) != set(rhs_cols):
497  
-                    break
498  
-
499  
-                cols = [lhs_cols[rhs_cols.index(col)] for col in cols]
500  
-                self.query.unref_alias(alias)
501  
-                alias = join.lhs_alias
502  
-        return cols, alias
  480
+        return field, targets, alias, joins, path, opts
503 481
 
504 482
     def get_from_clause(self):
505 483
         """
13  tests/queries/tests.py
@@ -25,7 +25,7 @@
25 25
     OneToOneCategory, NullableName, ProxyCategory, SingleObject, RelatedObject,
26 26
     ModelA, ModelB, ModelC, ModelD, Responsibility, Job, JobResponsibilities,
27 27
     BaseA, FK1, Identifier, Program, Channel, Page, Paragraph, Chapter, Book,
28  
-    MyObject, Order, OrderItem)
  28
+    MyObject, Order, OrderItem, SharedConnection)
29 29
 
30 30
 class BaseQuerysetTest(TestCase):
31 31
     def assertValueQuerysetEqual(self, qs, values):
@@ -2977,3 +2977,14 @@ def test_wrong_type_lookup(self):
2977 2977
         self.assertQuerysetEqual(
2978 2978
             ObjectB.objects.filter(objecta__in=[wrong_type]),
2979 2979
             [ob], lambda x: x)
  2980
+
  2981
+class Ticket14056Tests(TestCase):
  2982
+    def test_ticket_14056(self):
  2983
+        s1 = SharedConnection.objects.create(data='s1')
  2984
+        s2 = SharedConnection.objects.create(data='s2')
  2985
+        s3 = SharedConnection.objects.create(data='s3')
  2986
+        PointerA.objects.create(connection=s2)
  2987
+        self.assertQuerysetEqual(
  2988
+            SharedConnection.objects.order_by('pointera__connection', 'pk'),
  2989
+            [s1, s3, s2], lambda x: x
  2990
+        )

0 notes on commit 9054098

Please sign in to comment.
Something went wrong with that request. Please try again.