Depracate passing False to OrderBy's nulls_first and nulls_last.

[Gordon Wrigley]

Consider the following:

In [11]: [tv.published_at for tv in TemplateVersion.objects.order_by(F("published_at").desc(nulls_first=True))]
Out[11]: 
[None,
 datetime.datetime(2022, 2, 25, 13, 0, 12, 91916, tzinfo=<UTC>),
 datetime.datetime(2022, 2, 21, 10, 18, 0, 169248, tzinfo=<UTC>)]

In [12]: [tv.published_at for tv in TemplateVersion.objects.order_by(F("published_at").desc(nulls_first=False))]
Out[12]: 
[None,
 datetime.datetime(2022, 2, 25, 13, 0, 12, 91916, tzinfo=<UTC>),
 datetime.datetime(2022, 2, 21, 10, 18, 0, 169248, tzinfo=<UTC>)]

In [13]: [tv.published_at for tv in TemplateVersion.objects.order_by(F("published_at").desc(nulls_last=True))]
Out[13]: 
[datetime.datetime(2022, 2, 25, 13, 0, 12, 91916, tzinfo=<UTC>),
 datetime.datetime(2022, 2, 21, 10, 18, 0, 169248, tzinfo=<UTC>),
 None]

In [14]: [tv.published_at for tv in TemplateVersion.objects.order_by(F("published_at").desc(nulls_last=False))]
Out[14]: 
[None,
 datetime.datetime(2022, 2, 25, 13, 0, 12, 91916, tzinfo=<UTC>),
 datetime.datetime(2022, 2, 21, 10, 18, 0, 169248, tzinfo=<UTC>)]

Observe how nulls_first=False still puts the nulls first.

This happens because they both default False and when they are both False it lets the DB decide.

This is surprising behaviour, it also makes changing the null positioning based on a variable more awkward than it needs to be.

I think it would be better if they defaulted to None, let the DB decide when both are None and when one is not None do the ordering that implies.

[Adam Johnson]

I concur that it is confusing that nulls_first=False can still put the nulls first, and equally for the nulls_last=False.

I don't think we can change the semantics though as it would be a breaking change. The best we can probably do is make passing False for either a TypeError, by swapping their defaults for sentinel values.

Then, if you need a variable to switch between the behaviours you can use a construct like:

F(...).desc(**{("nulls_first" if nulls_first else "nulls_last"): True})

[Mariusz Felisiak]

Agreed, this can be confusing.

Replying to Adam Johnson:

I concur that it is confusing that nulls_first=False can still put the nulls first, and equally for the nulls_last=False.

I don't think we can change the semantics though as it would be a breaking change. The best we can probably do is make passing False for either a TypeError, by swapping their defaults for sentinel values.

Then, if you need a variable to switch between the behaviours you can use a construct like:

F(...).desc(**{("nulls_first" if nulls_first else "nulls_last"): True})

... or we could change defaults to None and deprecate passing False:

django/db/models/expressions.py

@@ -1357 +1357 @@
     conditional = False
 
     def __init__(
-        self, expression, descending=False, nulls_first=False, nulls_last=False
+        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:
+            warnings.warn(...)
         self.nulls_first = nulls_first
         self.nulls_last = nulls_last
         self.descending = descending
@@ -1427 +1429 @@
 
     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 is True:
+            self.nulls_first = None
+        if self.nulls_last is True:
+            self.nulls_last = None
         return self
 
     def asc(self):

[Mariusz Felisiak]

​PR

[Mariusz Felisiak]

​New PR

[GitHub <noreply@…>]

In 68da6b3:

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

Thanks Allen Jonathan David for the initial patch.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 94ad46e9:

Refs #33543 -- Made Expression.asc()/desc() and OrderBy raise ValueError when nulls_first/nulls_last=False is passed.

Per deprecation timeline.