Browse Source

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.
Nick Pope 3 years ago
parent
commit
fee8734596

+ 52 - 17
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

+ 4 - 0
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

+ 21 - 0
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
 ==================================
 

+ 7 - 0
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
 =======================
 

+ 46 - 1
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']})