CursorPagination doesn't process ordering from django-filters #9809
Unanswered
ulgens
asked this question in
Potential Issue
Replies: 0 comments
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
-
After migrating one of our projects to cursor pagination, we lost the ordering functionality in all related API endpoints. After a deep dive, I found that the related code block on
CursorPaginationclasshttps://github.com/encode/django-rest-framework/blob/main/rest_framework/pagination.py#L626
connected to the
.get_ordering()of the same class that is not compatible withdjango_filters.filters.OrderingFilter. That class applies the filtering via its own.filter()class and doesn't expose the selected ordering options to the outside, likerest_framework.filters.OrderingFilter.get_ordering. That missing interface leads toCursorPagination.paginate_queryset()overriding any ordering provided by django-filters.I tried to understand the goal here, and it seems the code is trying to guarantee that an ordering is present, but it does so without checking first.
GenericAPIView.filter_queryset()is already applying all the filters available, and thanks to that, I believeCursorPaginationcan have a little less customization and working integration with django-filters by having two changes:.get_ordering()is called only ifqueryset.orderedis false. If the goal is to enforce ordering, no need to apply an additional ordering to a queryset that is already ordered.GenericAPIView.filter_queryset(), and the enforced ordering is applied only when no other ordering is done (thanks to the previous step),CursorPagination.get_ordering()won't need to process ordering classes.An additional benefit of this approach is eliminating the issue defined in #9371 by completely removing the related code block.
The resulting set of changes looks like this: https://github.com/ulgens/django-rest-framework/pull/1/files
Beta Was this translation helpful? Give feedback.
All reactions