CursorPagination prevent to use default instance ordering attribute value if an ordering filter exist

[fdomain]

get_ordering method of CursorPagination returns the ordering by following this logic (see: https://github.com/encode/django-rest-framework/blob/master/rest_framework/pagination.py#L800):

• it checks if there is an ordering filter configured on the view, if it exists we get the value of this filter
• if there is no ordering filter, we fallback on the ordering attribute set on the instance (default being -created)

The problem is that if we have have a viewset defining an ordering filter like ordering=, but this attribute is unset in the query made by the client, current behavior of get_ordering will consider that the ordering is not defined and throw an exception (it doesn't fallback on the instance ordering attribute).

django-rest-framework/rest_framework/pagination.py

Line 815 in 38a74b4

assert ordering is not None, (

AssertionError: Using cursor pagination, but filter class OrderingFilter returned a `None` ordering.

My assumption is that we should reverse the logic and take first the value from self.ordering, and then look for ordering_filters (if any), and instead of throwing an exception if it returns None we just return the first value initialized (thus, self.ordering)

Here is what I would propose:

    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')
        ]

        if ordering_filters:
            # If a filter exists on the view that implements `get_ordering`
            # then we defer to that filter to determine the ordering.
            filter_cls = ordering_filters[0]
            filter_instance = filter_cls()
            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 and the filter class returned a `None` ordering.'
        )

        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(
                type=type(ordering).__name__
            )
        )

        if isinstance(ordering, str):
            return (ordering,)
        return tuple(ordering)

Unless I completely missed something obvious, I'd appreciate to have your opinion/feedback on this issue.
Thanks.

[auvipy]

we recently got a contribution in that area b1cec51 . can you check that the thing you are suggesting is still needed, and if so verify the changes with appropriate test cases?

[fdomain]

Thanks for your answer, and yes the commit you mentioned doesn't fix what I raised.
I'll push a change with appropriate test cases.