fix: fallback on CursorPagination ordering if unset on the view (#8954)

* this commit fixes the usage of a CursorPagination combined with a view
  implementing an ordering filter, without a default ordering value.
* former behavior was to fetch the ordering value from the filter, and
  raises an error if the value was None, preventing the fallback on the
  ordering set on the CursorPagination class itself.
* we reversed the logic by getting first the value set on the class,
  and override it by the ordering filter if the parameter is present

Author: fdomain

rest_framework/pagination.py

@@ -801,6 +801,10 @@ def get_ordering(self, request, queryset, view):
         """
         Return a tuple of strings, that may be used in an `order_by` method.
         """
+        # The default case is to check for an `ordering` attribute
+        # on this pagination instance.
+        ordering = self.ordering
+
         ordering_filters = [
             filter_cls for filter_cls in getattr(view, 'filter_backends', [])
             if hasattr(filter_cls, 'get_ordering')
@@ -811,26 +815,19 @@ def get_ordering(self, request, queryset, view):
             # then we defer to that filter to determine the ordering.
             filter_cls = ordering_filters[0]
             filter_instance = filter_cls()
-            ordering = filter_instance.get_ordering(request, queryset, view)
-            assert ordering is not None, (
-                'Using cursor pagination, but filter class {filter_cls} '
-                'returned a `None` ordering.'.format(
-                    filter_cls=filter_cls.__name__
-                )
-            )
-        else:
-            # The default case is to check for an `ordering` attribute
-            # on this pagination instance.
-            ordering = self.ordering
-            assert ordering is not None, (
-                'Using cursor pagination, but no ordering attribute was declared '
-                'on the pagination class.'
-            )
-            assert '__' not in ordering, (
-                'Cursor pagination does not support double underscore lookups '
-                'for orderings. Orderings should be an unchanging, unique or '
-                'nearly-unique field on the model, such as "-created" or "pk".'
-            )
+            ordering_from_filter = filter_instance.get_ordering(request, queryset, view)
+            if ordering_from_filter:
+                ordering = ordering_from_filter
+
+        assert ordering is not None, (
+            'Using cursor pagination, but no ordering attribute was declared '
+            'on the pagination class.'
+        )
+        assert '__' not in ordering, (
+            'Cursor pagination does not support double underscore lookups '
+            'for orderings. Orderings should be an unchanging, unique or '
+            'nearly-unique field on the model, such as "-created" or "pk".'
+        )
 
         assert isinstance(ordering, (str, list, tuple)), (
             'Invalid ordering. Expected string or tuple, but got {type}'.format(

tests/test_pagination.py

@@ -632,6 +632,24 @@ class MockView:
         ordering = self.pagination.get_ordering(request, [], MockView())
         assert ordering == ('created',)
 
+    def test_use_with_ordering_filter_without_ordering_default_value(self):
+        class MockView:
+            filter_backends = (filters.OrderingFilter,)
+            ordering_fields = ['username', 'created']
+
+        request = Request(factory.get('/'))
+        ordering = self.pagination.get_ordering(request, [], MockView())
+        # it gets the value of `ordering` provided by CursorPagination
+        assert ordering == ('created',)
+
+        request = Request(factory.get('/', {'ordering': 'username'}))
+        ordering = self.pagination.get_ordering(request, [], MockView())
+        assert ordering == ('username',)
+
+        request = Request(factory.get('/', {'ordering': 'invalid'}))
+        ordering = self.pagination.get_ordering(request, [], MockView())
+        assert ordering == ('created',)
+
     def test_cursor_pagination(self):
         (previous, current, next, previous_url, next_url) = self.get_pages('/')