Refs #14357 -- Deprecated Meta.ordering affecting GROUP BY queries.

Thanks Ramiro Morales for contributing to the patch.

django/db/models/sql/compiler.py

@@ -14,7 +14,9 @@ from django.db.models.sql.constants import (
 from django.db.models.sql.query import Query, get_order_dir
 from django.db.transaction import TransactionManagementError
 from django.db.utils import DatabaseError, NotSupportedError
-from django.utils.deprecation import RemovedInDjango30Warning
+from django.utils.deprecation import (
+    RemovedInDjango30Warning, RemovedInDjango31Warning,
+)
 from django.utils.inspect import func_supports_parameter
 
 FORCE = object()
@@ -34,6 +36,7 @@ class SQLCompiler:
         self.annotation_col_map = None
         self.klass_info = None
         self.ordering_parts = re.compile(r'(.*)\s(ASC|DESC)(.*)')
+        self._meta_ordering = None
 
     def setup_query(self):
         if all(self.query.alias_refcount[a] == 0 for a in self.query.alias_map):
@@ -266,8 +269,13 @@ class SQLCompiler:
             ordering = self.query.extra_order_by
         elif not self.query.default_ordering:
             ordering = self.query.order_by
+        elif self.query.order_by:
+            ordering = self.query.order_by
+        elif self.query.get_meta().ordering:
+            ordering = self.query.get_meta().ordering
+            self._meta_ordering = ordering
         else:
-            ordering = (self.query.order_by or self.query.get_meta().ordering or [])
+            ordering = []
         if self.query.standard_ordering:
             asc, desc = ORDER_DIR['ASC']
         else:
@@ -536,7 +544,18 @@ class SQLCompiler:
                         raise NotImplementedError('annotate() + distinct(fields) is not implemented.')
                     order_by = order_by or self.connection.ops.force_no_ordering()
                     result.append('GROUP BY %s' % ', '.join(grouping))
-
+                    if self._meta_ordering:
+                        # When the deprecation ends, replace with:
+                        # order_by = None
+                        warnings.warn(
+                            "%s QuerySet won't use Meta.ordering in Django 3.1. "
+                            "Add .order_by('%s') to retain the current query." % (
+                                self.query.model.__name__,
+                                "', '".join(self._meta_ordering)
+                            ),
+                            RemovedInDjango31Warning,
+                            stacklevel=4,
+                        )
                 if having:
                     result.append('HAVING %s' % having)
                     params.extend(h_params)

docs/internals/deprecation.txt

@@ -19,6 +19,8 @@ details on these changes.
 
 * ``django.core.paginator.QuerySetPaginator`` will be removed.
 
+* A model's ``Meta.ordering`` will no longer affect ``GROUP BY`` queries.
+
 .. _deprecation-removed-in-3.0:
 
 3.0

docs/ref/models/options.txt

@@ -285,7 +285,8 @@ Django quotes column and table names behind the scenes.
         ordering = [F('author').asc(nulls_last=True)]
 
     Default ordering also affects :ref:`aggregation queries
-    <aggregation-ordering-interaction>`.
+    <aggregation-ordering-interaction>` but this won't be the case starting
+    in Django 3.1.
 
 .. warning::
 

docs/releases/2.2.txt

@@ -289,6 +289,15 @@ Miscellaneous
 Features deprecated in 2.2
 ==========================
 
+Model ``Meta.ordering`` will no longer affect ``GROUP BY`` queries
+------------------------------------------------------------------
+
+A model's ``Meta.ordering`` affecting ``GROUP BY`` queries (such as
+``.annotate().values()``) is a common source of confusion. Such queries now
+issue a deprecation warning with the advice to add an ``order_by()`` to retain
+the current query. ``Meta.ordering`` will be ignored in such queries starting
+in Django 3.1.
+
 Miscellaneous
 -------------
 

docs/topics/db/aggregation.txt

@@ -514,6 +514,13 @@ include the aggregate column.
 Interaction with default ordering or ``order_by()``
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
+.. deprecated:: 2.2
+
+    Starting in Django 3.1, the ordering from a model's ``Meta.ordering`` won't
+    be used in ``GROUP BY`` queries, such as ``.annotate().values()``. Since
+    Django 2.2, these queries issue a deprecation warning indicating to add an
+    explicit ``order_by()`` to the queryset to silence the warning.
+
 Fields that are mentioned in the ``order_by()`` part of a queryset (or which
 are used in the default ordering on a model) are used when selecting the
 output data, even if they are not otherwise specified in the ``values()``

tests/aggregation_regress/tests.py

@@ -11,8 +11,11 @@ from django.db.models import (
     Avg, Case, Count, DecimalField, F, IntegerField, Max, Q, StdDev, Sum,
     Value, Variance, When,
 )
-from django.test import TestCase, skipUnlessAnyDBFeature, skipUnlessDBFeature
+from django.test import (
+    TestCase, ignore_warnings, skipUnlessAnyDBFeature, skipUnlessDBFeature,
+)
 from django.test.utils import Approximate
+from django.utils.deprecation import RemovedInDjango31Warning
 
 from .models import (
     Alfa, Author, Book, Bravo, Charlie, Clues, Entries, HardbackBook, ItemTag,
@@ -106,6 +109,7 @@ class AggregationTests(TestCase):
         for attr, value in kwargs.items():
             self.assertEqual(getattr(obj, attr), value)
 
+    @ignore_warnings(category=RemovedInDjango31Warning)
     def test_annotation_with_value(self):
         values = Book.objects.filter(
             name='Practical Django Projects',
@@ -213,6 +217,7 @@ class AggregationTests(TestCase):
             {'pages__sum': 3703}
         )
 
+    @ignore_warnings(category=RemovedInDjango31Warning)
     def test_annotation(self):
         # Annotations get combined with extra select clauses
         obj = Book.objects.annotate(mean_auth_age=Avg("authors__age")).extra(
@@ -306,6 +311,7 @@ class AggregationTests(TestCase):
 
         # If an annotation isn't included in the values, it can still be used
         # in a filter
+        with ignore_warnings(category=RemovedInDjango31Warning):
             qs = Book.objects.annotate(n_authors=Count('authors')).values('name').filter(n_authors__gt=2)
         self.assertSequenceEqual(
             qs, [
@@ -450,6 +456,7 @@ class AggregationTests(TestCase):
         with self.assertRaisesMessage(FieldError, msg):
             Book.objects.all().annotate(num_authors=Count('authors__id')).aggregate(Max('foo'))
 
+    @ignore_warnings(category=RemovedInDjango31Warning)
     def test_more(self):
         # Old-style count aggregations can be mixed with new-style
         self.assertEqual(
@@ -679,7 +686,7 @@ class AggregationTests(TestCase):
         )
 
         # Regression for #10127 - Empty select_related() works with annotate
-        qs = Book.objects.filter(rating__lt=4.5).select_related().annotate(Avg('authors__age'))
+        qs = Book.objects.filter(rating__lt=4.5).select_related().annotate(Avg('authors__age')).order_by('name')
         self.assertQuerysetEqual(
             qs,
             [
@@ -803,6 +810,7 @@ class AggregationTests(TestCase):
         with self.assertRaisesMessage(ValueError, msg):
             Author.objects.annotate(book_contact_set=Avg('friends__age'))
 
+    @ignore_warnings(category=RemovedInDjango31Warning)
     def test_pickle(self):
         # Regression for #10197 -- Queries with aggregates can be pickled.
         # First check that pickling is possible at all. No crash = success
@@ -933,7 +941,9 @@ class AggregationTests(TestCase):
             {'n_pages': 2078},
         )
 
-        qs = HardbackBook.objects.annotate(n_authors=Count('book_ptr__authors')).values('name', 'n_authors')
+        qs = HardbackBook.objects.annotate(
+            n_authors=Count('book_ptr__authors'),
+        ).values('name', 'n_authors').order_by('name')
         self.assertSequenceEqual(
             qs,
             [
@@ -945,7 +955,7 @@ class AggregationTests(TestCase):
             ],
         )
 
-        qs = HardbackBook.objects.annotate(n_authors=Count('authors')).values('name', 'n_authors')
+        qs = HardbackBook.objects.annotate(n_authors=Count('authors')).values('name', 'n_authors').order_by('name')
         self.assertSequenceEqual(
             qs,
             [
@@ -1005,7 +1015,7 @@ class AggregationTests(TestCase):
     def test_values_annotate_values(self):
         qs = Book.objects.values("name").annotate(
             n_authors=Count("authors")
-        ).values_list("pk", flat=True)
+        ).values_list("pk", flat=True).order_by('name')
         self.assertEqual(list(qs), list(Book.objects.values_list("pk", flat=True)))
 
     def test_having_group_by(self):
@@ -1015,7 +1025,7 @@ class AggregationTests(TestCase):
             n_authors=Count("authors")
         ).filter(
             pages__gt=F("n_authors")
-        ).values_list("name", flat=True)
+        ).values_list("name", flat=True).order_by('name')
         # Results should be the same, all Books have more pages than authors
         self.assertEqual(
             list(qs), list(Book.objects.values_list("name", flat=True))
@@ -1035,7 +1045,7 @@ class AggregationTests(TestCase):
     def test_annotation_disjunction(self):
         qs = Book.objects.annotate(n_authors=Count("authors")).filter(
             Q(n_authors=2) | Q(name="Python Web Development with Django")
-        )
+        ).order_by('name')
         self.assertQuerysetEqual(
             qs, [
                 "Artificial Intelligence: A Modern Approach",
@@ -1052,7 +1062,7 @@ class AggregationTests(TestCase):
                 Q(name="The Definitive Guide to Django: Web Development Done Right") |
                 (Q(name="Artificial Intelligence: A Modern Approach") & Q(n_authors=3))
             )
-        )
+        ).order_by('name')
         self.assertQuerysetEqual(
             qs,
             [
@@ -1200,6 +1210,7 @@ class AggregationTests(TestCase):
             {'book__count__max': 2}
         )
 
+    @ignore_warnings(category=RemovedInDjango31Warning)
     def test_annotate_joins(self):
         """
         The base table's join isn't promoted to LOUTER. This could
@@ -1436,6 +1447,7 @@ class AggregationTests(TestCase):
         query.filter(q1 | q2)
         self.assertEqual(len(q2.children), 1)
 
+    @ignore_warnings(category=RemovedInDjango31Warning)
     def test_fobj_group_by(self):
         """
         An F() object referring to related column works correctly in group by.
@@ -1513,6 +1525,7 @@ class JoinPromotionTests(TestCase):
         qs = Charlie.objects.annotate(Count('alfa__name'))
         self.assertIn(' LEFT OUTER JOIN ', str(qs.query))
 
+    @ignore_warnings(category=RemovedInDjango31Warning)
     def test_non_nullable_fk_not_promoted(self):
         qs = Book.objects.annotate(Count('contact__name'))
         self.assertIn(' INNER JOIN ', str(qs.query))

tests/ordering/tests.py

@@ -1,9 +1,10 @@
 from datetime import datetime
 from operator import attrgetter
 
-from django.db.models import DateTimeField, F, Max, OuterRef, Subquery
+from django.db.models import Count, DateTimeField, F, Max, OuterRef, Subquery
 from django.db.models.functions import Upper
 from django.test import TestCase
+from django.utils.deprecation import RemovedInDjango31Warning
 
 from .models import Article, Author, OrderedByFArticle, Reference
 
@@ -403,3 +404,11 @@ class OrderingTests(TestCase):
             articles, ['Article 1', 'Article 4', 'Article 3', 'Article 2'],
             attrgetter('headline')
         )
+
+    def test_deprecated_values_annotate(self):
+        msg = (
+            "Article QuerySet won't use Meta.ordering in Django 3.1. Add "
+            ".order_by('-pub_date', 'headline') to retain the current query."
+        )
+        with self.assertRaisesMessage(RemovedInDjango31Warning, msg):
+            list(Article.objects.values('author').annotate(Count('headline')))

tests/queries/test_explain.py

@@ -2,8 +2,11 @@ import unittest
 
 from django.db import NotSupportedError, connection, transaction
 from django.db.models import Count
-from django.test import TestCase, skipIfDBFeature, skipUnlessDBFeature
+from django.test import (
+    TestCase, ignore_warnings, skipIfDBFeature, skipUnlessDBFeature,
+)
 from django.test.utils import CaptureQueriesContext
+from django.utils.deprecation import RemovedInDjango31Warning
 
 from .models import Tag
 
@@ -11,6 +14,7 @@ from .models import Tag
 @skipUnlessDBFeature('supports_explaining_query_execution')
 class ExplainTests(TestCase):
 
+    @ignore_warnings(category=RemovedInDjango31Warning)
     def test_basic(self):
         querysets = [
             Tag.objects.filter(name='test'),

tests/queries/tests.py

@@ -1156,7 +1156,7 @@ class Queries1Tests(TestCase):
     def test_ticket_20250(self):
         # A negated Q along with an annotated queryset failed in Django 1.4
         qs = Author.objects.annotate(Count('item'))
-        qs = qs.filter(~Q(extra__value=0))
+        qs = qs.filter(~Q(extra__value=0)).order_by('name')
 
         self.assertIn('SELECT', str(qs.query))
         self.assertQuerysetEqual(