Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Last ordering filter has the last say #9371

Closed
wants to merge 1 commit into from
Closed

Conversation

tuky
Copy link
Contributor

@tuky tuky commented Apr 8, 2024

Description

In

for backend in list(self.filter_backends):
every filter backend is called in the order as listed in the view's filter_backends. And since every order_by qs call clears the previous order (https://docs.djangoproject.com/en/5.0/ref/models/querysets/#django.db.models.query.QuerySet.order_by), the last ordering filter should have the final say for cursor pagination.

Although a bug fix, this poses the risk of being a major change for dependent projects.

PS:
IMO, the docs should have a warning somewhere about this. It seems uncommon or unexpected from DRF's side to have multiple ordering backends and even more unexpected with cursor pagination where you have to be extra careful anyways. And there should be a warning that CursorPagination only uses ordering[0] for the cursor although it would be possible to implement a cursor on multiple fields a la Q(ordering0__gt=cursor0) | Q(ordering0__gte=cursor0, ordering1__gt=cursor1) | ... (unfolded result QS).

@tomchristie
Copy link
Member

this poses the risk of being a major change for dependent projects.

So... our default has to be that we wouldn't accept changes like this.

Furthermore we really need to push towards a high cutoff bar for accepting changes, and should be ensuring we've got really clear contribution guidelines that enforce a process of...

  • Discussions first.
  • Escalate to issues only if accepted by the team.
  • Pull requests only once there's an associated issue.

@tomchristie tomchristie closed this Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants