Ensure CursorPagination respects nulls in the ordering field (#8912)

* Ensure CursorPagination respects nulls in the ordering field

* Lint

* Fix pagination tests

* Add test_ascending with nulls

* Push tests for nulls

* Test pass

* Add comment

* Fix test for django30

Author: ddelange

rest_framework/pagination.py

@@ -10,6 +10,7 @@
 
 from django.core.paginator import InvalidPage
 from django.core.paginator import Paginator as DjangoPaginator
+from django.db.models import Q
 from django.template import loader
 from django.utils.encoding import force_str
 from django.utils.translation import gettext_lazy as _
@@ -620,7 +621,7 @@ def paginate_queryset(self, queryset, request, view=None):
             queryset = queryset.order_by(*self.ordering)
 
         # If we have a cursor with a fixed position then filter by that.
-        if current_position is not None:
+        if str(current_position) != 'None':
             order = self.ordering[0]
             is_reversed = order.startswith('-')
             order_attr = order.lstrip('-')
@@ -631,7 +632,12 @@ def paginate_queryset(self, queryset, request, view=None):
             else:
                 kwargs = {order_attr + '__gt': current_position}
 
-            queryset = queryset.filter(**kwargs)
+            filter_query = Q(**kwargs)
+            # If some records contain a null for the ordering field, don't lose them.
+            # When reverse ordering, nulls will come last and need to be included.
+            if (reverse and not is_reversed) or is_reversed:
+                filter_query |= Q(**{order_attr + '__isnull': True})
+            queryset = queryset.filter(filter_query)
 
         # If we have an offset cursor then offset the entire page by that amount.
         # We also always fetch an extra item in order to determine if there is a
@@ -704,7 +710,7 @@ def get_next_link(self):
                 # The item in this position and the item following it
                 # have different positions. We can use this position as
                 # our marker.
-                has_item_with_unique_position = True
+                has_item_with_unique_position = position is not None
                 break
 
             # The item in this position has the same position as the item
@@ -757,7 +763,7 @@ def get_previous_link(self):
                 # The item in this position and the item following it
                 # have different positions. We can use this position as
                 # our marker.
-                has_item_with_unique_position = True
+                has_item_with_unique_position = position is not None
                 break
 
             # The item in this position has the same position as the item
@@ -883,7 +889,7 @@ def _get_position_from_instance(self, instance, ordering):
             attr = instance[field_name]
         else:
             attr = getattr(instance, field_name)
-        return str(attr)
+        return None if attr is None else str(attr)
 
     def get_paginated_response(self, data):
         return Response(OrderedDict([

tests/test_pagination.py

@@ -951,17 +951,24 @@ class MockQuerySet:
             def __init__(self, items):
                 self.items = items
 
-            def filter(self, created__gt=None, created__lt=None):
+            def filter(self, q):
+                q_args = dict(q.deconstruct()[1])
+                if not q_args:
+                    # django 3.0.x artifact
+                    q_args = dict(q.deconstruct()[2])
+                created__gt = q_args.get('created__gt')
+                created__lt = q_args.get('created__lt')
+
                 if created__gt is not None:
                     return MockQuerySet([
                         item for item in self.items
-                        if item.created > int(created__gt)
+                        if item.created is None or item.created > int(created__gt)
                     ])
 
                 assert created__lt is not None
                 return MockQuerySet([
                     item for item in self.items
-                    if item.created < int(created__lt)
+                    if item.created is None or item.created < int(created__lt)
                 ])
 
             def order_by(self, *ordering):
@@ -1080,6 +1087,127 @@ def get_pages(self, url):
         return (previous, current, next, previous_url, next_url)
 
 
+class NullableCursorPaginationModel(models.Model):
+    created = models.IntegerField(null=True)
+
+
+class TestCursorPaginationWithNulls(TestCase):
+    """
+    Unit tests for `pagination.CursorPagination` with ordering on a nullable field.
+    """
+
+    def setUp(self):
+        class ExamplePagination(pagination.CursorPagination):
+            page_size = 1
+            ordering = 'created'
+
+        self.pagination = ExamplePagination()
+        data = [
+            None, None, 3, 4
+        ]
+        for idx in data:
+            NullableCursorPaginationModel.objects.create(created=idx)
+
+        self.queryset = NullableCursorPaginationModel.objects.all()
+
+    get_pages = TestCursorPagination.get_pages
+
+    def test_ascending(self):
+        """Test paginating one row at a time, current should go 1, 2, 3, 4, 3, 2, 1."""
+        (previous, current, next, previous_url, next_url) = self.get_pages('/')
+
+        assert previous is None
+        assert current == [None]
+        assert next == [None]
+
+        (previous, current, next, previous_url, next_url) = self.get_pages(next_url)
+
+        assert previous == [None]
+        assert current == [None]
+        assert next == [3]
+
+        (previous, current, next, previous_url, next_url) = self.get_pages(next_url)
+
+        assert previous == [3]  # [None] paging artifact documented at https://github.com/ddelange/django-rest-framework/blob/3.14.0/rest_framework/pagination.py#L789
+        assert current == [3]
+        assert next == [4]
+
+        (previous, current, next, previous_url, next_url) = self.get_pages(next_url)
+
+        assert previous == [3]
+        assert current == [4]
+        assert next is None
+        assert next_url is None
+
+        (previous, current, next, previous_url, next_url) = self.get_pages(previous_url)
+
+        assert previous == [None]
+        assert current == [3]
+        assert next == [4]
+
+        (previous, current, next, previous_url, next_url) = self.get_pages(previous_url)
+
+        assert previous == [None]
+        assert current == [None]
+        assert next == [None]  # [3] paging artifact documented at https://github.com/ddelange/django-rest-framework/blob/3.14.0/rest_framework/pagination.py#L731
+
+        (previous, current, next, previous_url, next_url) = self.get_pages(previous_url)
+
+        assert previous is None
+        assert current == [None]
+        assert next == [None]
+
+    def test_descending(self):
+        """Test paginating one row at a time, current should go 4, 3, 2, 1, 2, 3, 4."""
+        self.pagination.ordering = ('-created',)
+        (previous, current, next, previous_url, next_url) = self.get_pages('/')
+
+        assert previous is None
+        assert current == [4]
+        assert next == [3]
+
+        (previous, current, next, previous_url, next_url) = self.get_pages(next_url)
+
+        assert previous == [None]  # [4] paging artifact
+        assert current == [3]
+        assert next == [None]
+
+        (previous, current, next, previous_url, next_url) = self.get_pages(next_url)
+
+        assert previous == [None]  # [3] paging artifact
+        assert current == [None]
+        assert next == [None]
+
+        (previous, current, next, previous_url, next_url) = self.get_pages(next_url)
+
+        assert previous == [None]
+        assert current == [None]
+        assert next is None
+        assert next_url is None
+
+        (previous, current, next, previous_url, next_url) = self.get_pages(previous_url)
+
+        assert previous == [3]
+        assert current == [None]
+        assert next == [None]
+
+        (previous, current, next, previous_url, next_url) = self.get_pages(previous_url)
+
+        assert previous == [None]
+        assert current == [3]
+        assert next == [3]  # [4] paging artifact documented at https://github.com/ddelange/django-rest-framework/blob/3.14.0/rest_framework/pagination.py#L731
+
+        # skip back artifact
+        (previous, current, next, previous_url, next_url) = self.get_pages(previous_url)
+        (previous, current, next, previous_url, next_url) = self.get_pages(previous_url)
+
+        (previous, current, next, previous_url, next_url) = self.get_pages(previous_url)
+
+        assert previous is None
+        assert current == [4]
+        assert next == [3]
+
+
 def test_get_displayed_page_numbers():
     """
     Test our contextual page display function.