From fee87345967b3d917b618533585076cbfa43451b Mon Sep 17 00:00:00 2001 From: Nick Pope Date: Wed, 30 Jun 2021 00:08:27 +0100 Subject: [PATCH] Refs #10929 -- Deprecated forced empty result value for PostgreSQL aggregates. This deprecates forcing a return value for ArrayAgg, JSONBAgg, and StringAgg when there are no rows in the query. Now that we have a ``default`` argument for aggregates, we want to revert to returning the default of ``None`` which most aggregate functions return and leave it up to the user to decide what they want to be returned by default. --- django/contrib/postgres/aggregates/general.py | 69 ++++++++++++++----- docs/internals/deprecation.txt | 4 ++ docs/ref/contrib/postgres/aggregates.txt | 21 ++++++ docs/releases/4.0.txt | 7 ++ tests/postgres_tests/test_aggregates.py | 47 ++++++++++++- 5 files changed, 130 insertions(+), 18 deletions(-) diff --git a/django/contrib/postgres/aggregates/general.py b/django/contrib/postgres/aggregates/general.py index d36f71fddf..ae432a016c 100644 --- a/django/contrib/postgres/aggregates/general.py +++ b/django/contrib/postgres/aggregates/general.py @@ -1,5 +1,8 @@ +import warnings + from django.contrib.postgres.fields import ArrayField from django.db.models import Aggregate, BooleanField, JSONField, Value +from django.utils.deprecation import RemovedInDjango50Warning from .mixins import OrderableAggMixin @@ -8,20 +11,44 @@ __all__ = [ ] -class ArrayAgg(OrderableAggMixin, Aggregate): +# RemovedInDjango50Warning +NOT_PROVIDED = object() + + +class DeprecatedConvertValueMixin: + def __init__(self, *expressions, default=NOT_PROVIDED, **extra): + if default is NOT_PROVIDED: + default = None + self._default_provided = False + else: + self._default_provided = True + super().__init__(*expressions, default=default, **extra) + + def convert_value(self, value, expression, connection): + if value is None and not self._default_provided: + warnings.warn(self.deprecation_msg, category=RemovedInDjango50Warning) + return self.deprecation_value + return value + + +class ArrayAgg(DeprecatedConvertValueMixin, OrderableAggMixin, Aggregate): function = 'ARRAY_AGG' template = '%(function)s(%(distinct)s%(expressions)s %(ordering)s)' allow_distinct = True + # RemovedInDjango50Warning + deprecation_value = property(lambda self: []) + deprecation_msg = ( + 'In Django 5.0, ArrayAgg() will return None instead of an empty list ' + 'if there are no rows. Pass default=None to opt into the new behavior ' + 'and silence this warning or default=Value([]) to keep the previous ' + 'behavior.' + ) + @property def output_field(self): return ArrayField(self.source_expressions[0].output_field) - def convert_value(self, value, expression, connection): - if value is None and self.default is None: - return [] - return value - class BitAnd(Aggregate): function = 'BIT_AND' @@ -41,28 +68,36 @@ class BoolOr(Aggregate): output_field = BooleanField() -class JSONBAgg(OrderableAggMixin, Aggregate): +class JSONBAgg(DeprecatedConvertValueMixin, OrderableAggMixin, Aggregate): function = 'JSONB_AGG' template = '%(function)s(%(distinct)s%(expressions)s %(ordering)s)' allow_distinct = True output_field = JSONField() - def convert_value(self, value, expression, connection): - if value is None and self.default is None: - return '[]' - return value + # RemovedInDjango50Warning + deprecation_value = '[]' + deprecation_msg = ( + "In Django 5.0, JSONBAgg() will return None instead of an empty list " + "if there are no rows. Pass default=None to opt into the new behavior " + "and silence this warning or default=Value('[]') to keep the previous " + "behavior." + ) -class StringAgg(OrderableAggMixin, Aggregate): +class StringAgg(DeprecatedConvertValueMixin, OrderableAggMixin, Aggregate): function = 'STRING_AGG' template = '%(function)s(%(distinct)s%(expressions)s %(ordering)s)' allow_distinct = True + # RemovedInDjango50Warning + deprecation_value = '' + deprecation_msg = ( + "In Django 5.0, StringAgg() will return None instead of an empty " + "string if there are no rows. Pass default=None to opt into the new " + "behavior and silence this warning or default=Value('') to keep the " + "previous behavior." + ) + def __init__(self, expression, delimiter, **extra): delimiter_expr = Value(str(delimiter)) super().__init__(expression, delimiter_expr, **extra) - - def convert_value(self, value, expression, connection): - if value is None and self.default is None: - return '' - return value diff --git a/docs/internals/deprecation.txt b/docs/internals/deprecation.txt index c56b875aed..16801e0345 100644 --- a/docs/internals/deprecation.txt +++ b/docs/internals/deprecation.txt @@ -30,6 +30,10 @@ details on these changes. * The ``extra_tests`` argument for ``DiscoverRunner.build_suite()`` and ``DiscoverRunner.run_tests()`` will be removed. +* The ``django.contrib.postgres.aggregates.ArrayAgg``, ``JSONBAgg``, and + ``StringAgg`` aggregates will return ``None`` when there are no rows instead + of ``[]``, ``[]``, and ``''`` respectively. + .. _deprecation-removed-in-4.1: 4.1 diff --git a/docs/ref/contrib/postgres/aggregates.txt b/docs/ref/contrib/postgres/aggregates.txt index 61ec86fa44..6d85d5c478 100644 --- a/docs/ref/contrib/postgres/aggregates.txt +++ b/docs/ref/contrib/postgres/aggregates.txt @@ -52,6 +52,13 @@ General-purpose aggregation functions from django.db.models import F F('some_field').desc() + .. deprecated:: 4.0 + + If there are no rows and ``default`` is not provided, ``ArrayAgg`` + returns an empty list instead of ``None``. This behavior is deprecated + and will be removed in Django 5.0. If you need it, explicitly set + ``default`` to ``Value([])``. + ``BitAnd`` ---------- @@ -138,6 +145,13 @@ General-purpose aggregation functions Examples are the same as for :attr:`ArrayAgg.ordering`. + .. deprecated:: 4.0 + + If there are no rows and ``default`` is not provided, ``JSONBAgg`` + returns an empty list instead of ``None``. This behavior is deprecated + and will be removed in Django 5.0. If you need it, explicitly set + ``default`` to ``Value('[]')``. + ``StringAgg`` ------------- @@ -164,6 +178,13 @@ General-purpose aggregation functions Examples are the same as for :attr:`ArrayAgg.ordering`. + .. deprecated:: 4.0 + + If there are no rows and ``default`` is not provided, ``StringAgg`` + returns an empty string instead of ``None``. This behavior is + deprecated and will be removed in Django 5.0. If you need it, + explicitly set ``default`` to ``Value('')``. + Aggregate functions for statistics ================================== diff --git a/docs/releases/4.0.txt b/docs/releases/4.0.txt index 4122c9b419..244f113c54 100644 --- a/docs/releases/4.0.txt +++ b/docs/releases/4.0.txt @@ -540,6 +540,13 @@ Miscellaneous * The ``extra_tests`` argument for :meth:`.DiscoverRunner.build_suite` and :meth:`.DiscoverRunner.run_tests` is deprecated. +* The :class:`~django.contrib.postgres.aggregates.ArrayAgg`, + :class:`~django.contrib.postgres.aggregates.JSONBAgg`, and + :class:`~django.contrib.postgres.aggregates.StringAgg` aggregates will return + ``None`` when there are no rows instead of ``[]``, ``[]``, and ``''`` + respectively in Django 5.0. If you need the previous behavior, explicitly set + ``default`` to ``Value([])``, ``Value('[]')``, or ``Value('')``. + Features removed in 4.0 ======================= diff --git a/tests/postgres_tests/test_aggregates.py b/tests/postgres_tests/test_aggregates.py index 393e3d38e8..7ae7b16c9f 100644 --- a/tests/postgres_tests/test_aggregates.py +++ b/tests/postgres_tests/test_aggregates.py @@ -3,7 +3,8 @@ from django.db.models import ( ) from django.db.models.fields.json import KeyTextTransform, KeyTransform from django.db.models.functions import Cast, Concat, Substr -from django.test.utils import Approximate +from django.test.utils import Approximate, ignore_warnings +from django.utils.deprecation import RemovedInDjango50Warning from . import PostgreSQLTestCase from .models import AggregateTestModel, StatTestModel @@ -44,6 +45,7 @@ class TestGeneralAggregate(PostgreSQLTestCase): ), ]) + @ignore_warnings(category=RemovedInDjango50Warning) def test_empty_result_set(self): AggregateTestModel.objects.all().delete() tests = [ @@ -100,6 +102,49 @@ class TestGeneralAggregate(PostgreSQLTestCase): ) self.assertEqual(values, {'aggregation': expected_result}) + def test_convert_value_deprecation(self): + AggregateTestModel.objects.all().delete() + queryset = AggregateTestModel.objects.all() + + with self.assertWarnsMessage(RemovedInDjango50Warning, ArrayAgg.deprecation_msg): + queryset.aggregate(aggregation=ArrayAgg('boolean_field')) + + with self.assertWarnsMessage(RemovedInDjango50Warning, JSONBAgg.deprecation_msg): + queryset.aggregate(aggregation=JSONBAgg('integer_field')) + + with self.assertWarnsMessage(RemovedInDjango50Warning, StringAgg.deprecation_msg): + queryset.aggregate(aggregation=StringAgg('char_field', delimiter=';')) + + # No warnings raised if default argument provided. + self.assertEqual( + queryset.aggregate(aggregation=ArrayAgg('boolean_field', default=None)), + {'aggregation': None}, + ) + self.assertEqual( + queryset.aggregate(aggregation=JSONBAgg('integer_field', default=None)), + {'aggregation': None}, + ) + self.assertEqual( + queryset.aggregate( + aggregation=StringAgg('char_field', delimiter=';', default=None), + ), + {'aggregation': None}, + ) + self.assertEqual( + queryset.aggregate(aggregation=ArrayAgg('boolean_field', default=Value([]))), + {'aggregation': []}, + ) + self.assertEqual( + queryset.aggregate(aggregation=JSONBAgg('integer_field', default=Value('[]'))), + {'aggregation': []}, + ) + self.assertEqual( + queryset.aggregate( + aggregation=StringAgg('char_field', delimiter=';', default=Value('')), + ), + {'aggregation': ''}, + ) + def test_array_agg_charfield(self): values = AggregateTestModel.objects.aggregate(arrayagg=ArrayAgg('char_field')) self.assertEqual(values, {'arrayagg': ['Foo1', 'Foo2', 'Foo4', 'Foo3']})