From d734f1651ccc0a74325f7b55f7eecc68edef6453 Mon Sep 17 00:00:00 2001 From: Chris Muthig Date: Sat, 6 Jul 2024 10:44:11 -0600 Subject: [PATCH] Refs #35444 -- Deprecated contrib.postgres aggregates ordering for order_by. Aligns the argument with SQL standards already used in Window.order_by and sets up for adding support to Aggregate. --- django/contrib/postgres/aggregates/general.py | 6 +- django/contrib/postgres/aggregates/mixins.py | 33 ++++-- docs/internals/deprecation.txt | 5 + docs/ref/contrib/postgres/aggregates.txt | 41 +++++-- docs/releases/3.2.txt | 2 +- docs/releases/5.2.txt | 6 + tests/postgres_tests/test_aggregates.py | 109 +++++++++++------- 7 files changed, 137 insertions(+), 65 deletions(-) diff --git a/django/contrib/postgres/aggregates/general.py b/django/contrib/postgres/aggregates/general.py index 5de53676f6..48968b51b8 100644 --- a/django/contrib/postgres/aggregates/general.py +++ b/django/contrib/postgres/aggregates/general.py @@ -17,7 +17,7 @@ __all__ = [ class ArrayAgg(OrderableAggMixin, Aggregate): function = "ARRAY_AGG" - template = "%(function)s(%(distinct)s%(expressions)s %(ordering)s)" + template = "%(function)s(%(distinct)s%(expressions)s %(order_by)s)" allow_distinct = True @property @@ -49,14 +49,14 @@ class BoolOr(Aggregate): class JSONBAgg(OrderableAggMixin, Aggregate): function = "JSONB_AGG" - template = "%(function)s(%(distinct)s%(expressions)s %(ordering)s)" + template = "%(function)s(%(distinct)s%(expressions)s %(order_by)s)" allow_distinct = True output_field = JSONField() class StringAgg(OrderableAggMixin, Aggregate): function = "STRING_AGG" - template = "%(function)s(%(distinct)s%(expressions)s %(ordering)s)" + template = "%(function)s(%(distinct)s%(expressions)s %(order_by)s)" allow_distinct = True output_field = TextField() diff --git a/django/contrib/postgres/aggregates/mixins.py b/django/contrib/postgres/aggregates/mixins.py index d6ff535158..6af43a7fea 100644 --- a/django/contrib/postgres/aggregates/mixins.py +++ b/django/contrib/postgres/aggregates/mixins.py @@ -1,15 +1,30 @@ +import warnings + from django.core.exceptions import FullResultSet from django.db.models.expressions import OrderByList +from django.utils.deprecation import RemovedInDjango61Warning class OrderableAggMixin: - def __init__(self, *expressions, ordering=(), **extra): - if not ordering: + # RemovedInDjango61Warning: When the deprecation ends, replace with: + # def __init__(self, *expressions, order_by=(), **extra): + def __init__(self, *expressions, ordering=(), order_by=(), **extra): + # RemovedInDjango61Warning. + if ordering: + warnings.warn( + "The ordering argument is deprecated. Use order_by instead.", + category=RemovedInDjango61Warning, + stacklevel=2, + ) + if order_by: + raise TypeError("Cannot specify both order_by and ordering.") + order_by = ordering + if not order_by: self.order_by = None - elif isinstance(ordering, (list, tuple)): - self.order_by = OrderByList(*ordering) + elif isinstance(order_by, (list, tuple)): + self.order_by = OrderByList(*order_by) else: - self.order_by = OrderByList(ordering) + self.order_by = OrderByList(order_by) super().__init__(*expressions, **extra) def resolve_expression(self, *args, **kwargs): @@ -25,12 +40,12 @@ class OrderableAggMixin: return super().set_source_expressions(exprs) def as_sql(self, compiler, connection): - *source_exprs, filtering_expr, ordering_expr = self.get_source_expressions() + *source_exprs, filtering_expr, order_by_expr = self.get_source_expressions() order_by_sql = "" order_by_params = [] - if ordering_expr is not None: - order_by_sql, order_by_params = compiler.compile(ordering_expr) + if order_by_expr is not None: + order_by_sql, order_by_params = compiler.compile(order_by_expr) filter_params = [] if filtering_expr is not None: @@ -43,5 +58,5 @@ class OrderableAggMixin: for source_expr in source_exprs: source_params += compiler.compile(source_expr)[1] - sql, _ = super().as_sql(compiler, connection, ordering=order_by_sql) + sql, _ = super().as_sql(compiler, connection, order_by=order_by_sql) return sql, (*source_params, *order_by_params, *filter_params) diff --git a/docs/internals/deprecation.txt b/docs/internals/deprecation.txt index 171d9ecbe3..71e4e19cd8 100644 --- a/docs/internals/deprecation.txt +++ b/docs/internals/deprecation.txt @@ -22,6 +22,11 @@ details on these changes. ``django.contrib.auth.login()`` and ``django.contrib.auth.alogin()`` will be removed. +* The ``ordering`` keyword argument of the PostgreSQL specific aggregation + functions ``django.contrib.postgres.aggregates.ArrayAgg``, + ``django.contrib.postgres.aggregates.JSONBAgg``, and + ``django.contrib.postgres.aggregates.StringAgg`` will be removed. + .. _deprecation-removed-in-6.0: 6.0 diff --git a/docs/ref/contrib/postgres/aggregates.txt b/docs/ref/contrib/postgres/aggregates.txt index fd4fabe853..e9d6de5d74 100644 --- a/docs/ref/contrib/postgres/aggregates.txt +++ b/docs/ref/contrib/postgres/aggregates.txt @@ -30,7 +30,7 @@ General-purpose aggregation functions ``ArrayAgg`` ------------ -.. class:: ArrayAgg(expression, distinct=False, filter=None, default=None, ordering=(), **extra) +.. class:: ArrayAgg(expression, distinct=False, filter=None, default=None, order_by=(), **extra) Returns a list of values, including nulls, concatenated into an array, or ``default`` if there are no values. @@ -40,7 +40,9 @@ General-purpose aggregation functions An optional boolean argument that determines if array values will be distinct. Defaults to ``False``. - .. attribute:: ordering + .. attribute:: order_by + + .. versionadded:: 5.2 An optional string of a field name (with an optional ``"-"`` prefix which indicates descending order) or an expression (or a tuple or list @@ -55,6 +57,11 @@ General-purpose aggregation functions F("some_field").desc() + .. deprecated:: 5.2 + + The ``ordering`` keyword argument is deprecated. Use + :attr:`ArrayAgg.order_by` instead. + ``BitAnd`` ---------- @@ -130,7 +137,7 @@ General-purpose aggregation functions ``JSONBAgg`` ------------ -.. class:: JSONBAgg(expressions, distinct=False, filter=None, default=None, ordering=(), **extra) +.. class:: JSONBAgg(expressions, distinct=False, filter=None, default=None, order_by=(), **extra) Returns the input values as a ``JSON`` array, or ``default`` if there are no values. You can query the result using :lookup:`key and index lookups @@ -141,14 +148,16 @@ General-purpose aggregation functions An optional boolean argument that determines if array values will be distinct. Defaults to ``False``. - .. attribute:: ordering + .. attribute:: order_by + + .. versionadded:: 5.2 An optional string of a field name (with an optional ``"-"`` prefix which indicates descending order) or an expression (or a tuple or list of strings and/or expressions) that specifies the ordering of the elements in the result list. - Examples are the same as for :attr:`ArrayAgg.ordering`. + Examples are the same as for :attr:`ArrayAgg.order_by`. Usage example:: @@ -168,7 +177,7 @@ General-purpose aggregation functions >>> Room.objects.annotate( ... requirements=JSONBAgg( ... "hotelreservation__requirements", - ... ordering="-hotelreservation__start", + ... order_by="-hotelreservation__start", ... ) ... ).filter(requirements__0__sea_view=True).values("number", "requirements") + .. deprecated:: 5.2 + + The ``ordering`` keyword argument is deprecated. Use + :attr:`JSONBAgg.order_by` instead. + ``StringAgg`` ------------- -.. class:: StringAgg(expression, delimiter, distinct=False, filter=None, default=None, ordering=()) +.. class:: StringAgg(expression, delimiter, distinct=False, filter=None, default=None, order_by=()) Returns the input values concatenated into a string, separated by the ``delimiter`` string, or ``default`` if there are no values. @@ -193,14 +207,16 @@ General-purpose aggregation functions An optional boolean argument that determines if concatenated values will be distinct. Defaults to ``False``. - .. attribute:: ordering + .. attribute:: order_by + + .. versionadded:: 5.2 An optional string of a field name (with an optional ``"-"`` prefix which indicates descending order) or an expression (or a tuple or list of strings and/or expressions) that specifies the ordering of the elements in the result string. - Examples are the same as for :attr:`ArrayAgg.ordering`. + Examples are the same as for :attr:`ArrayAgg.order_by`. Usage example:: @@ -224,13 +240,18 @@ General-purpose aggregation functions ... publication_names=StringAgg( ... "publications__title", ... delimiter=", ", - ... ordering="publications__title", + ... order_by="publications__title", ... ) ... ).values("headline", "publication_names") + .. deprecated:: 5.2 + + The ``ordering`` keyword argument is deprecated. Use + :attr:`StringAgg.order_by` instead. + Aggregate functions for statistics ================================== diff --git a/docs/releases/3.2.txt b/docs/releases/3.2.txt index 55bfff0940..0b695ef19e 100644 --- a/docs/releases/3.2.txt +++ b/docs/releases/3.2.txt @@ -250,7 +250,7 @@ Minor features * The new ``ExclusionConstraint.opclasses`` attribute allows setting PostgreSQL operator classes. -* The new :attr:`.JSONBAgg.ordering` attribute determines the ordering of the +* The new ``JSONBAgg.ordering`` attribute determines the ordering of the aggregated elements. * The new :attr:`.JSONBAgg.distinct` attribute determines if aggregated values diff --git a/docs/releases/5.2.txt b/docs/releases/5.2.txt index 58e92f756d..69899b0554 100644 --- a/docs/releases/5.2.txt +++ b/docs/releases/5.2.txt @@ -495,3 +495,9 @@ Miscellaneous * The fallback to ``request.user`` when ``user`` is ``None`` in ``django.contrib.auth.login()`` and ``django.contrib.auth.alogin()`` will be removed. + +* The ``ordering`` keyword argument of the PostgreSQL specific aggregation + functions ``django.contrib.postgres.aggregates.ArrayAgg``, + ``django.contrib.postgres.aggregates.JSONBAgg``, and + ``django.contrib.postgres.aggregates.StringAgg`` is deprecated in favor + of the ``order_by`` argument. diff --git a/tests/postgres_tests/test_aggregates.py b/tests/postgres_tests/test_aggregates.py index b72310bdf1..885512cfca 100644 --- a/tests/postgres_tests/test_aggregates.py +++ b/tests/postgres_tests/test_aggregates.py @@ -15,6 +15,7 @@ from django.db.models.fields.json import KeyTextTransform, KeyTransform from django.db.models.functions import Cast, Concat, LPad, Substr from django.test.utils import Approximate from django.utils import timezone +from django.utils.deprecation import RemovedInDjango61Warning from . import PostgreSQLTestCase from .models import AggregateTestModel, HotelReservation, Room, StatTestModel @@ -148,12 +149,36 @@ class TestGeneralAggregate(PostgreSQLTestCase): ) self.assertEqual(values, {"aggregation": expected_result}) + def test_ordering_warns_of_deprecation(self): + msg = "The ordering argument is deprecated. Use order_by instead." + with self.assertWarnsMessage(RemovedInDjango61Warning, msg) as ctx: + values = AggregateTestModel.objects.aggregate( + arrayagg=ArrayAgg("integer_field", ordering=F("integer_field").desc()) + ) + self.assertEqual(values, {"arrayagg": [2, 1, 0, 0]}) + self.assertEqual(ctx.filename, __file__) + + def test_ordering_and_order_by_causes_error(self): + with self.assertWarns(RemovedInDjango61Warning): + with self.assertRaisesMessage( + TypeError, + "Cannot specify both order_by and ordering.", + ): + AggregateTestModel.objects.aggregate( + stringagg=StringAgg( + "char_field", + delimiter=Value("'"), + order_by="char_field", + ordering="char_field", + ) + ) + def test_array_agg_charfield(self): values = AggregateTestModel.objects.aggregate(arrayagg=ArrayAgg("char_field")) self.assertEqual(values, {"arrayagg": ["Foo1", "Foo2", "Foo4", "Foo3"]}) - def test_array_agg_charfield_ordering(self): - ordering_test_cases = ( + def test_array_agg_charfield_order_by(self): + order_by_test_cases = ( (F("char_field").desc(), ["Foo4", "Foo3", "Foo2", "Foo1"]), (F("char_field").asc(), ["Foo1", "Foo2", "Foo3", "Foo4"]), (F("char_field"), ["Foo1", "Foo2", "Foo3", "Foo4"]), @@ -178,10 +203,10 @@ class TestGeneralAggregate(PostgreSQLTestCase): ["Foo3", "Foo1", "Foo2", "Foo4"], ), ) - for ordering, expected_output in ordering_test_cases: - with self.subTest(ordering=ordering, expected_output=expected_output): + for order_by, expected_output in order_by_test_cases: + with self.subTest(order_by=order_by, expected_output=expected_output): values = AggregateTestModel.objects.aggregate( - arrayagg=ArrayAgg("char_field", ordering=ordering) + arrayagg=ArrayAgg("char_field", order_by=order_by) ) self.assertEqual(values, {"arrayagg": expected_output}) @@ -191,9 +216,9 @@ class TestGeneralAggregate(PostgreSQLTestCase): ) self.assertEqual(values, {"arrayagg": [0, 1, 2, 0]}) - def test_array_agg_integerfield_ordering(self): + def test_array_agg_integerfield_order_by(self): values = AggregateTestModel.objects.aggregate( - arrayagg=ArrayAgg("integer_field", ordering=F("integer_field").desc()) + arrayagg=ArrayAgg("integer_field", order_by=F("integer_field").desc()) ) self.assertEqual(values, {"arrayagg": [2, 1, 0, 0]}) @@ -203,16 +228,16 @@ class TestGeneralAggregate(PostgreSQLTestCase): ) self.assertEqual(values, {"arrayagg": [True, False, False, True]}) - def test_array_agg_booleanfield_ordering(self): - ordering_test_cases = ( + def test_array_agg_booleanfield_order_by(self): + order_by_test_cases = ( (F("boolean_field").asc(), [False, False, True, True]), (F("boolean_field").desc(), [True, True, False, False]), (F("boolean_field"), [False, False, True, True]), ) - for ordering, expected_output in ordering_test_cases: - with self.subTest(ordering=ordering, expected_output=expected_output): + for order_by, expected_output in order_by_test_cases: + with self.subTest(order_by=order_by, expected_output=expected_output): values = AggregateTestModel.objects.aggregate( - arrayagg=ArrayAgg("boolean_field", ordering=ordering) + arrayagg=ArrayAgg("boolean_field", order_by=order_by) ) self.assertEqual(values, {"arrayagg": expected_output}) @@ -225,22 +250,22 @@ class TestGeneralAggregate(PostgreSQLTestCase): ) self.assertEqual(values, {"arrayagg": ["pl", "en"]}) - def test_array_agg_jsonfield_ordering(self): + def test_array_agg_jsonfield_order_by(self): values = AggregateTestModel.objects.aggregate( arrayagg=ArrayAgg( KeyTransform("lang", "json_field"), filter=Q(json_field__lang__isnull=False), - ordering=KeyTransform("lang", "json_field"), + order_by=KeyTransform("lang", "json_field"), ), ) self.assertEqual(values, {"arrayagg": ["en", "pl"]}) - def test_array_agg_filter_and_ordering_params(self): + def test_array_agg_filter_and_order_by_params(self): values = AggregateTestModel.objects.aggregate( arrayagg=ArrayAgg( "char_field", filter=Q(json_field__has_key="lang"), - ordering=LPad(Cast("integer_field", CharField()), 2, Value("0")), + order_by=LPad(Cast("integer_field", CharField()), 2, Value("0")), ) ) self.assertEqual(values, {"arrayagg": ["Foo2", "Foo4"]}) @@ -406,8 +431,8 @@ class TestGeneralAggregate(PostgreSQLTestCase): ) self.assertEqual(values, {"stringagg": "Text1;Text2;Text4;Text3"}) - def test_string_agg_charfield_ordering(self): - ordering_test_cases = ( + def test_string_agg_charfield_order_by(self): + order_by_test_cases = ( (F("char_field").desc(), "Foo4;Foo3;Foo2;Foo1"), (F("char_field").asc(), "Foo1;Foo2;Foo3;Foo4"), (F("char_field"), "Foo1;Foo2;Foo3;Foo4"), @@ -416,19 +441,19 @@ class TestGeneralAggregate(PostgreSQLTestCase): (Concat("char_field", Value("@")), "Foo1;Foo2;Foo3;Foo4"), (Concat("char_field", Value("@")).desc(), "Foo4;Foo3;Foo2;Foo1"), ) - for ordering, expected_output in ordering_test_cases: - with self.subTest(ordering=ordering, expected_output=expected_output): + for order_by, expected_output in order_by_test_cases: + with self.subTest(order_by=order_by, expected_output=expected_output): values = AggregateTestModel.objects.aggregate( - stringagg=StringAgg("char_field", delimiter=";", ordering=ordering) + stringagg=StringAgg("char_field", delimiter=";", order_by=order_by) ) self.assertEqual(values, {"stringagg": expected_output}) - def test_string_agg_jsonfield_ordering(self): + def test_string_agg_jsonfield_order_by(self): values = AggregateTestModel.objects.aggregate( stringagg=StringAgg( KeyTextTransform("lang", "json_field"), delimiter=";", - ordering=KeyTextTransform("lang", "json_field"), + order_by=KeyTextTransform("lang", "json_field"), output_field=CharField(), ), ) @@ -446,7 +471,7 @@ class TestGeneralAggregate(PostgreSQLTestCase): def test_orderable_agg_alternative_fields(self): values = AggregateTestModel.objects.aggregate( - arrayagg=ArrayAgg("integer_field", ordering=F("char_field").asc()) + arrayagg=ArrayAgg("integer_field", order_by=F("char_field").asc()) ) self.assertEqual(values, {"arrayagg": [0, 1, 0, 2]}) @@ -454,8 +479,8 @@ class TestGeneralAggregate(PostgreSQLTestCase): values = AggregateTestModel.objects.aggregate(jsonbagg=JSONBAgg("char_field")) self.assertEqual(values, {"jsonbagg": ["Foo1", "Foo2", "Foo4", "Foo3"]}) - def test_jsonb_agg_charfield_ordering(self): - ordering_test_cases = ( + def test_jsonb_agg_charfield_order_by(self): + order_by_test_cases = ( (F("char_field").desc(), ["Foo4", "Foo3", "Foo2", "Foo1"]), (F("char_field").asc(), ["Foo1", "Foo2", "Foo3", "Foo4"]), (F("char_field"), ["Foo1", "Foo2", "Foo3", "Foo4"]), @@ -464,38 +489,38 @@ class TestGeneralAggregate(PostgreSQLTestCase): (Concat("char_field", Value("@")), ["Foo1", "Foo2", "Foo3", "Foo4"]), (Concat("char_field", Value("@")).desc(), ["Foo4", "Foo3", "Foo2", "Foo1"]), ) - for ordering, expected_output in ordering_test_cases: - with self.subTest(ordering=ordering, expected_output=expected_output): + for order_by, expected_output in order_by_test_cases: + with self.subTest(order_by=order_by, expected_output=expected_output): values = AggregateTestModel.objects.aggregate( - jsonbagg=JSONBAgg("char_field", ordering=ordering), + jsonbagg=JSONBAgg("char_field", order_by=order_by), ) self.assertEqual(values, {"jsonbagg": expected_output}) - def test_jsonb_agg_integerfield_ordering(self): + def test_jsonb_agg_integerfield_order_by(self): values = AggregateTestModel.objects.aggregate( - jsonbagg=JSONBAgg("integer_field", ordering=F("integer_field").desc()), + jsonbagg=JSONBAgg("integer_field", order_by=F("integer_field").desc()), ) self.assertEqual(values, {"jsonbagg": [2, 1, 0, 0]}) - def test_jsonb_agg_booleanfield_ordering(self): - ordering_test_cases = ( + def test_jsonb_agg_booleanfield_order_by(self): + order_by_test_cases = ( (F("boolean_field").asc(), [False, False, True, True]), (F("boolean_field").desc(), [True, True, False, False]), (F("boolean_field"), [False, False, True, True]), ) - for ordering, expected_output in ordering_test_cases: - with self.subTest(ordering=ordering, expected_output=expected_output): + for order_by, expected_output in order_by_test_cases: + with self.subTest(order_by=order_by, expected_output=expected_output): values = AggregateTestModel.objects.aggregate( - jsonbagg=JSONBAgg("boolean_field", ordering=ordering), + jsonbagg=JSONBAgg("boolean_field", order_by=order_by), ) self.assertEqual(values, {"jsonbagg": expected_output}) - def test_jsonb_agg_jsonfield_ordering(self): + def test_jsonb_agg_jsonfield_order_by(self): values = AggregateTestModel.objects.aggregate( jsonbagg=JSONBAgg( KeyTransform("lang", "json_field"), filter=Q(json_field__lang__isnull=False), - ordering=KeyTransform("lang", "json_field"), + order_by=KeyTransform("lang", "json_field"), ), ) self.assertEqual(values, {"jsonbagg": ["en", "pl"]}) @@ -533,7 +558,7 @@ class TestGeneralAggregate(PostgreSQLTestCase): Room.objects.annotate( requirements=JSONBAgg( "hotelreservation__requirements", - ordering="-hotelreservation__start", + order_by="-hotelreservation__start", ) ) .filter(requirements__0__sea_view=True) @@ -552,7 +577,7 @@ class TestGeneralAggregate(PostgreSQLTestCase): ], ) - def test_string_agg_array_agg_ordering_in_subquery(self): + def test_string_agg_array_agg_order_by_in_subquery(self): stats = [] for i, agg in enumerate(AggregateTestModel.objects.order_by("char_field")): stats.append(StatTestModel(related_field=agg, int1=i, int2=i + 1)) @@ -561,7 +586,7 @@ class TestGeneralAggregate(PostgreSQLTestCase): for aggregate, expected_result in ( ( - ArrayAgg("stattestmodel__int1", ordering="-stattestmodel__int2"), + ArrayAgg("stattestmodel__int1", order_by="-stattestmodel__int2"), [ ("Foo1", [0, 1]), ("Foo2", [1, 2]), @@ -573,7 +598,7 @@ class TestGeneralAggregate(PostgreSQLTestCase): StringAgg( Cast("stattestmodel__int1", CharField()), delimiter=";", - ordering="-stattestmodel__int2", + order_by="-stattestmodel__int2", ), [("Foo1", "0;1"), ("Foo2", "1;2"), ("Foo3", "2;3"), ("Foo4", "3;4")], ),