Fixed #33543 -- Deprecated passing nulls_first/nulls_last=False to OrderBy and Expression.asc()/desc().

felixxm · web-flow · commit 68da6b389c40 · 2022-05-12T11:30:03.000+02:00
Thanks Allen Jonathan David for the initial patch.
diff --git a/django/db/models/expressions.py b/django/db/models/expressions.py
@@ -2,6 +2,7 @@
 import datetime
 import functools
 import inspect
+import warnings
 from collections import defaultdict
 from decimal import Decimal
 from uuid import UUID
@@ -12,6 +13,7 @@
 from django.db.models.constants import LOOKUP_SEP
 from django.db.models.query_utils import Q
 from django.utils.deconstruct import deconstructible
+from django.utils.deprecation import RemovedInDjango50Warning
 from django.utils.functional import cached_property
 from django.utils.hashable import make_hashable
 
@@ -1513,11 +1515,20 @@ class OrderBy(Expression):
     template = "%(expression)s %(ordering)s"
     conditional = False
 
-    def __init__(
-        self, expression, descending=False, nulls_first=False, nulls_last=False
-    ):
+    def __init__(self, expression, descending=False, nulls_first=None, nulls_last=None):
         if nulls_first and nulls_last:
             raise ValueError("nulls_first and nulls_last are mutually exclusive")
+        if nulls_first is False or nulls_last is False:
+            # When the deprecation ends, replace with:
+            # raise ValueError(
+            #     "nulls_first and nulls_last values must be True or None."
+            # )
+            warnings.warn(
+                "Passing nulls_first=False or nulls_last=False is deprecated, use None "
+                "instead.",
+                RemovedInDjango50Warning,
+                stacklevel=2,
+            )
         self.nulls_first = nulls_first
         self.nulls_last = nulls_last
         self.descending = descending
@@ -1584,9 +1595,12 @@ def get_group_by_cols(self, alias=None):
 
     def reverse_ordering(self):
         self.descending = not self.descending
-        if self.nulls_first or self.nulls_last:
-            self.nulls_first = not self.nulls_first
-            self.nulls_last = not self.nulls_last
+        if self.nulls_first:
+            self.nulls_last = True
+            self.nulls_first = None
+        elif self.nulls_last:
+            self.nulls_first = True
+            self.nulls_last = None
         return self
 
     def asc(self):
diff --git a/docs/internals/deprecation.txt b/docs/internals/deprecation.txt
@@ -105,6 +105,10 @@ details on these changes.
 
 * The ``django.contrib.auth.hashers.CryptPasswordHasher`` will be removed.
 
+* The ability to pass ``nulls_first=False`` or ``nulls_last=False`` to
+  ``Expression.asc()`` and ``Expression.desc()`` methods, and the ``OrderBy``
+  expression will be removed.
+
 .. _deprecation-removed-in-4.1:
 
 4.1
diff --git a/docs/ref/models/expressions.txt b/docs/ref/models/expressions.txt
@@ -1033,20 +1033,40 @@ calling the appropriate methods on the wrapped expression.
         to a column. The ``alias`` parameter will be ``None`` unless the
         expression has been annotated and is used for grouping.
 
-    .. method:: asc(nulls_first=False, nulls_last=False)
+    .. method:: asc(nulls_first=None, nulls_last=None)
 
         Returns the expression ready to be sorted in ascending order.
 
         ``nulls_first`` and ``nulls_last`` define how null values are sorted.
         See :ref:`using-f-to-sort-null-values` for example usage.
 
-    .. method:: desc(nulls_first=False, nulls_last=False)
+        .. versionchanged:: 4.1
+
+            In older versions, ``nulls_first`` and ``nulls_last`` defaulted to
+            ``False``.
+
+        .. deprecated:: 4.1
+
+            Passing ``nulls_first=False`` or ``nulls_last=False`` to ``asc()``
+            is deprecated. Use ``None`` instead.
+
+    .. method:: desc(nulls_first=None, nulls_last=None)
 
         Returns the expression ready to be sorted in descending order.
 
         ``nulls_first`` and ``nulls_last`` define how null values are sorted.
         See :ref:`using-f-to-sort-null-values` for example usage.
 
+        .. versionchanged:: 4.1
+
+            In older versions, ``nulls_first`` and ``nulls_last`` defaulted to
+            ``False``.
+
+        .. deprecated:: 4.1
+
+            Passing ``nulls_first=False`` or ``nulls_last=False`` to ``desc()``
+            is deprecated. Use ``None`` instead.
+
     .. method:: reverse_ordering()
 
         Returns ``self`` with any modifications required to reverse the sort
diff --git a/docs/releases/4.1.txt b/docs/releases/4.1.txt
@@ -685,6 +685,10 @@ Miscellaneous
 
 * ``django.contrib.auth.hashers.CryptPasswordHasher`` is deprecated.
 
+* The ability to pass ``nulls_first=False`` or ``nulls_last=False`` to
+  ``Expression.asc()`` and ``Expression.desc()`` methods, and the ``OrderBy``
+  expression is deprecated. Use ``None`` instead.
+
 Features removed in 4.1
 =======================
 
diff --git a/tests/expressions/tests.py b/tests/expressions/tests.py
@@ -69,6 +69,7 @@
     isolate_apps,
     register_lookup,
 )
+from django.utils.deprecation import RemovedInDjango50Warning
 from django.utils.functional import SimpleLazyObject
 
 from .models import (
@@ -2537,7 +2538,7 @@ def test_equal(self):
         )
         self.assertNotEqual(
             OrderBy(F("field"), nulls_last=True),
-            OrderBy(F("field"), nulls_last=False),
+            OrderBy(F("field")),
         )
 
     def test_hash(self):
@@ -2547,5 +2548,22 @@ def test_hash(self):
         )
         self.assertNotEqual(
             hash(OrderBy(F("field"), nulls_last=True)),
-            hash(OrderBy(F("field"), nulls_last=False)),
+            hash(OrderBy(F("field"))),
         )
+
+    def test_nulls_false(self):
+        # These tests will catch ValueError in Django 5.0 when passing False to
+        # nulls_first and nulls_last becomes forbidden.
+        # msg = "nulls_first and nulls_last values must be True or None."
+        msg = (
+            "Passing nulls_first=False or nulls_last=False is deprecated, use None "
+            "instead."
+        )
+        with self.assertRaisesMessage(RemovedInDjango50Warning, msg):
+            OrderBy(F("field"), nulls_first=False)
+        with self.assertRaisesMessage(RemovedInDjango50Warning, msg):
+            OrderBy(F("field"), nulls_last=False)
+        with self.assertRaisesMessage(RemovedInDjango50Warning, msg):
+            F("field").asc(nulls_first=False)
+        with self.assertRaisesMessage(RemovedInDjango50Warning, msg):
+            F("field").desc(nulls_last=False)
