Inconsistent pagination when sorting by non-unique columns

[alextatarinov]

As described in https://code.djangoproject.com/ticket/34251, some database backends (incl. PostgreSQL) will produce non-deterministic ordering when sorting by columns with non-unique values.

The standard DRF setup with OrderingFilter and PageNumber/LimitOffsetPagination invites this problem, and I've been bitten by it several times. The OrderingFilter abstraction makes you think that everything should work just fine with i.e. ordering_fields = ['name', 'created_date', 'price'].

Is easy to overlook, since such non-unique columns like "name" and "price" can have mostly unique values, and duplicate values have to be at the page borders to observe the issue, so it's more likely to happen in production environments after operating fine for quite a while. Moreover, unless you have a "load more" UI (which was my case), it's not easy to spot duplicates or missing rows across pages.

The current solution requires passing "ordering=field_name,pk" from the API's consumers, which in my opinion is less than ideal.

An alternative option is to enforce total ordering automatically, and this is what Django admin does in contrib/admin/views/main.py#L390 by inserting a PK when ordering by non-unique fields. The simple implementation can look like this

class StableOrderingFilter(OrderingFilter):
    def get_ordering(self, request, queryset, view):
        ordering = super().get_ordering(request, queryset, view)
        return list(ordering or []) + ['pk']

This solution can lead to another problem, stated in this Django ticket, which is unused indexes due to ordering by multiple columns.

I believe this issue affects quite a lot of production systems out there and requires our attention. The behavior is definitely unexpected and hard to notice, while sorting by arbitrary non-unique columns is a commonly used pattern. To my shock, I haven't seen this issue described or discussed pretty much anywhere, meaning almost every DRF project which uses ordering + pagination is affected.

If the linked ticket is accepted, Django will issue a warning in such scenarios, which will propagate to the DRF's PageNumberPagination, but not LimitOffsetPagination (which, on a side note, will also happily accept completely unordered QuerySet). But I don't think the warning is enough since there is currently no reasonable solution from the DRF side.

[auvipy]

lets wait for a consensus on django first.

[auvipy]

I am open for any improvement suggestion

[carltongibson]

I don't see it as a blocker.

It is if the consensus is that this isn't something that should be addressed.

lets wait for a consensus on django first.

+1 👍

[carltongibson]

On a more proactive take, if it were me, and I wanted this, I would create the necessary subclasses (and start using them) so that I could show exactly what the required change would look like.

(Code always progresses the conversation faster.)

[alextatarinov]

The StableOrderingFilter class in the first message is actually taken from a real system and it worked quite well.

On second thought, I'm thinking about optional behavior, allowing one to specify whether or not and which fields should be used to ensure stable ordering. Paired with documentation and mention in the tutorial, that should be a good enough backward-compatible solution. Roughly:

class StableOrderingFilter(OrderingFilter):
    def get_ordering(self, request, queryset, view):
        ordering = super().get_ordering(request, queryset, view)
        unique_ordering_fields = getattr(view, 'unique_ordering_fields', None)
        if unique_ordering_fields:
            return list(ordering or []) + unique_ordering_fields
        return ordering

[alextatarinov]

Based on the discussion there is no consensus to add a warning or enforce stable ordering automatically as Django admin does.

As such, I would like to suggest a new feature - a convenient way for developers to deal with unstable ordering - an optional base_ordering setting on a ViewSet, which, if provided, would always be added by OrderingFilter to whatever ordering was determined by existing rules.

As outlined in the previous message, the implementation can look something like this:

class StableOrderingFilter(OrderingFilter):
    def get_ordering(self, request, queryset, view):
        ordering = super().get_ordering(request, queryset, view)
        base_ordering = getattr(view, 'base_ordering', None)
        if ordering and base_ordering:
            ordering += [field for field in base_ordering if field not in ordering]
        return ordering

Should I go forward and prepare a PR to discuss the code more closely?