Revert "Ensure CursorPagination respects nulls in the ordering field (#…

…8912)" (#9381)

This reverts commit b1cec51.

rest_framework/pagination.py

@@ -11,7 +11,6 @@
 
 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 _
@@ -631,7 +630,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 str(current_position) != 'None':
+ if current_position is not None:
  order = self.ordering[0]
  is_reversed = order.startswith('-')
  order_attr = order.lstrip('-')
@@ -642,12 +641,7 @@ def paginate_queryset(self, queryset, request, view=None):
  else:
  kwargs = {order_attr + '__gt': current_position}
 
- 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)
+ queryset = queryset.filter(**kwargs)
 
  # 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
@@ -720,7 +714,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 = position is not None
+ has_item_with_unique_position = True
  break
 
  # The item in this position has the same position as the item
@@ -773,7 +767,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 = position is not None
+ has_item_with_unique_position = True
  break
 
  # The item in this position has the same position as the item
@@ -896,7 +890,7 @@ def _get_position_from_instance(self, instance, ordering):
  attr = instance[field_name]
  else:
  attr = getattr(instance, field_name)
- return None if attr is None else str(attr)
+ return str(attr)
 
  def get_paginated_response(self, data):
  return Response({

tests/test_pagination.py

@@ -972,24 +972,17 @@ class MockQuerySet:
  def __init__(self, items):
  self.items = items
 
- 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')
-
+ def filter(self, created__gt=None, created__lt=None):
  if created__gt is not None:
  return MockQuerySet([
  item for item in self.items
- if item.created is None or item.created > int(created__gt)
+ if item.created > int(created__gt)
  ])
 
  assert created__lt is not None
  return MockQuerySet([
  item for item in self.items
- if item.created is None or item.created < int(created__lt)
+ if item.created < int(created__lt)
  ])
 
  def order_by(self, *ordering):
@@ -1108,127 +1101,6 @@ 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.