Skip to content
This repository

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse code

Fixed #10113 -- Ensured that joined fields mentioned in order_by() ar…

…e included in the GROUP_BY clause on those backends that require it. Thanks to Koen Biermans <koen.biermans@werk.belgie.be> for the report.

git-svn-id: http://code.djangoproject.com/svn/django/trunk@9788 bcc190cf-cafb-0310-a4f2-bffc1f526a37
  • Loading branch information...
commit 15b1675af988d2918b142e8a0c2ab6fcf50e4649 1 parent f5ed27c
Russell Keith-Magee authored January 24, 2009
24  django/db/models/sql/query.py
@@ -346,7 +346,7 @@ def as_sql(self, with_limits=True, with_col_aliases=False):
346 346
         """
347 347
         self.pre_sql_setup()
348 348
         out_cols = self.get_columns(with_col_aliases)
349  
-        ordering = self.get_ordering()
  349
+        ordering, ordering_group_by = self.get_ordering()
350 350
 
351 351
         # This must come after 'select' and 'ordering' -- see docstring of
352 352
         # get_from_clause() for details.
@@ -380,9 +380,15 @@ def as_sql(self, with_limits=True, with_col_aliases=False):
380 380
 
381 381
         if self.group_by:
382 382
             grouping = self.get_grouping()
383  
-            result.append('GROUP BY %s' % ', '.join(grouping))
384  
-            if not ordering:
  383
+            if ordering:
  384
+                # If the backend can't group by PK (i.e., any database
  385
+                # other than MySQL), then any fields mentioned in the
  386
+                # ordering clause needs to be in the group by clause.
  387
+                if not self.connection.features.allows_group_by_pk:
  388
+                    grouping.extend(ordering_group_by)
  389
+            else:
385 390
                 ordering = self.connection.ops.force_no_ordering()
  391
+            result.append('GROUP BY %s' % ', '.join(grouping))
386 392
 
387 393
         if having:
388 394
             result.append('HAVING %s' % having)
@@ -701,7 +707,10 @@ def get_grouping(self):
701 707
 
702 708
     def get_ordering(self):
703 709
         """
704  
-        Returns list representing the SQL elements in the "order by" clause.
  710
+        Returns a tuple containing a list representing the SQL elements in the
  711
+        "order by" clause, and the list of SQL elements that need to be added
  712
+        to the GROUP BY clause as a result of the ordering.
  713
+
705 714
         Also sets the ordering_aliases attribute on this instance to a list of
706 715
         extra aliases needed in the select.
707 716
 
@@ -719,6 +728,7 @@ def get_ordering(self):
719 728
         distinct = self.distinct
720 729
         select_aliases = self._select_aliases
721 730
         result = []
  731
+        group_by = []
722 732
         ordering_aliases = []
723 733
         if self.standard_ordering:
724 734
             asc, desc = ORDER_DIR['ASC']
@@ -741,6 +751,7 @@ def get_ordering(self):
741 751
                 else:
742 752
                     order = asc
743 753
                 result.append('%s %s' % (field, order))
  754
+                group_by.append(field)
744 755
                 continue
745 756
             col, order = get_order_dir(field, asc)
746 757
             if col in self.aggregate_select:
@@ -755,6 +766,7 @@ def get_ordering(self):
755 766
                     processed_pairs.add((table, col))
756 767
                     if not distinct or elt in select_aliases:
757 768
                         result.append('%s %s' % (elt, order))
  769
+                        group_by.append(elt)
758 770
             elif get_order_dir(field)[0] not in self.extra_select:
759 771
                 # 'col' is of the form 'field' or 'field1__field2' or
760 772
                 # '-field1__field2__field', etc.
@@ -766,13 +778,15 @@ def get_ordering(self):
766 778
                         if distinct and elt not in select_aliases:
767 779
                             ordering_aliases.append(elt)
768 780
                         result.append('%s %s' % (elt, order))
  781
+                        group_by.append(elt)
769 782
             else:
770 783
                 elt = qn2(col)
771 784
                 if distinct and col not in select_aliases:
772 785
                     ordering_aliases.append(elt)
773 786
                 result.append('%s %s' % (elt, order))
  787
+                group_by.append(elt)
774 788
         self.ordering_aliases = ordering_aliases
775  
-        return result
  789
+        return result, group_by
776 790
 
777 791
     def find_ordering_name(self, name, opts, alias=None, default_order='ASC',
778 792
             already_seen=None):
5  tests/regressiontests/aggregation_regress/models.py
@@ -174,6 +174,11 @@ def __unicode__(self):
174 174
 >>> Publisher.objects.filter(pk=5).annotate(num_authors=Count('book__authors'), avg_authors=Avg('book__authors'), max_authors=Max('book__authors'), max_price=Max('book__price'), max_rating=Max('book__rating')).values()
175 175
 [{'max_authors': None, 'name': u"Jonno's House of Books", 'num_awards': 0, 'max_price': None, 'num_authors': 0, 'max_rating': None, 'id': 5, 'avg_authors': None}]
176 176
 
  177
+# Regression for #10113 - Fields mentioned in order_by() must be included in the GROUP BY.
  178
+# This only becomes a problem when the order_by introduces a new join.
  179
+>>> Book.objects.annotate(num_authors=Count('authors')).order_by('publisher__name')
  180
+[<Book: The Definitive Guide to Django: Web Development Done Right>, <Book: Practical Django Projects>, <Book: Paradigms of Artificial Intelligence Programming: Case Studies in Common Lisp>, <Book: Python Web Development with Django>, <Book: Artificial Intelligence: A Modern Approach>, <Book: Sams Teach Yourself Django in 24 Hours>]
  181
+
177 182
 """
178 183
 }
179 184
 

0 notes on commit 15b1675

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