diff options
| -rw-r--r-- | django/db/models/sql/compiler.py | 25 | ||||
| -rw-r--r-- | docs/internals/deprecation.txt | 2 | ||||
| -rw-r--r-- | docs/ref/models/options.txt | 3 | ||||
| -rw-r--r-- | docs/releases/2.2.txt | 9 | ||||
| -rw-r--r-- | docs/topics/db/aggregation.txt | 7 | ||||
| -rw-r--r-- | tests/aggregation_regress/tests.py | 31 | ||||
| -rw-r--r-- | tests/ordering/tests.py | 11 | ||||
| -rw-r--r-- | tests/queries/test_explain.py | 6 | ||||
| -rw-r--r-- | tests/queries/tests.py | 2 |
9 files changed, 80 insertions, 16 deletions
diff --git a/django/db/models/sql/compiler.py b/django/db/models/sql/compiler.py index f08082c88a..aa9ffc7b0e 100644 --- a/django/db/models/sql/compiler.py +++ b/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) diff --git a/docs/internals/deprecation.txt b/docs/internals/deprecation.txt index d7b02fa3bc..7043391527 100644 --- a/docs/internals/deprecation.txt +++ b/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 diff --git a/docs/ref/models/options.txt b/docs/ref/models/options.txt index 246f3e7d9a..3994e408a5 100644 --- a/docs/ref/models/options.txt +++ b/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:: diff --git a/docs/releases/2.2.txt b/docs/releases/2.2.txt index af2910a514..bf5bc30cde 100644 --- a/docs/releases/2.2.txt +++ b/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 ------------- diff --git a/docs/topics/db/aggregation.txt b/docs/topics/db/aggregation.txt index 50a92a5dbe..c709c064a5 100644 --- a/docs/topics/db/aggregation.txt +++ b/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()`` diff --git a/tests/aggregation_regress/tests.py b/tests/aggregation_regress/tests.py index 7f03d66bab..893b22ae69 100644 --- a/tests/aggregation_regress/tests.py +++ b/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,7 +311,8 @@ class AggregationTests(TestCase): # If an annotation isn't included in the values, it can still be used # in a filter - qs = Book.objects.annotate(n_authors=Count('authors')).values('name').filter(n_authors__gt=2) + with ignore_warnings(category=RemovedInDjango31Warning): + qs = Book.objects.annotate(n_authors=Count('authors')).values('name').filter(n_authors__gt=2) self.assertSequenceEqual( qs, [ {"name": 'Python Web Development with Django'} @@ -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)) diff --git a/tests/ordering/tests.py b/tests/ordering/tests.py index 8c07a27428..16e5cc9b2d 100644 --- a/tests/ordering/tests.py +++ b/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'))) diff --git a/tests/queries/test_explain.py b/tests/queries/test_explain.py index ad4ca988ee..9428bd88e9 100644 --- a/tests/queries/test_explain.py +++ b/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'), diff --git a/tests/queries/tests.py b/tests/queries/tests.py index 384bda4c77..65917f84fb 100644 --- a/tests/queries/tests.py +++ b/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( |