Warn about importance of ordering for all pagination classes#8494
Warn about importance of ordering for all pagination classes#8494Elkasitu wants to merge 3 commits intoencode:mainfrom
Conversation
The ordering recommendations given for the CursorPagination scheme actually apply to all pagination schemes, an unsuspecting developer that implements the more common `LimitOffsetPagination` or `PageNumberPagination` classes is unlikely to be aware of the importance of consistent ordering. This commit moves the `Details and limitations` section out of the `CursorPagination` section and puts it as the very first subsection of the `Pagination` page so that it's one of the first things that developers see. Some examples of inconsistencies as well as how to deal with them are given, and an extra way to change the ordering of a paginated view is provided. Fixes encode#6886
|
I'm not convinced that this section makes sense when moved around like this. For example from a first look over - it discusses the |
|
You're right, when I wrote this patch I hadn't actually tried to implement I have made some changes, I kept it as a general section because the only Pagination class that is really different from the others is |
|
Segue sempre sem fazer nenhuma alteração |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
@tomchristie any chance of ever getting something like this merged? I feel like it's an easy and dangerous trap to fall into |
did you addressed the concerns @tomchristie initially raised? |
|
@auvipy Well there was only one concern which was about the If there are any other concerns I'm willing to address them too |
|
|
||
| If the main field that you wish to order by does not satisfy these conditions, you can order by multiple fields, as long as one of the fields fulfills all of the conditions above the result set should remain consistent across database calls. | ||
|
|
||
| # inconsistent |
There was a problem hiding this comment.
I think it would be better to include for elaborate code example here including the pagination classes
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
Still looks relevant. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
I cannot edit the PR, but the following examples can be added; the lack of a unique, unchanging ordering field can cause records to shift between pages during navigation, especially if the name or status fields change or have duplicate values. from django.db import models
from rest_framework import pagination, serializers, viewsets
# --- INCONSISTENT EXAMPLES ---
class Product(models.Model):
name = models.CharField(max_length=100)
category = models.CharField(max_length=100)
# No unique/indexed field used for ordering
class ProductSerializer(serializers.ModelSerializer):
class Meta:
model = Product
fields = '__all__'
# 1. Inconsistent LimitOffsetPagination
class UnreliableProductViewSet(viewsets.ModelViewSet):
queryset = Product.objects.all().order_by('category') # Many duplicates, inconsistent pages
serializer_class = ProductSerializer
pagination_class = pagination.LimitOffsetPagination
# 2. Inconsistent CursorPagination (Will fail or be buggy)
class RiskyCursorPagination(pagination.CursorPagination):
page_size = 10
ordering = 'name' # Problematic: 'name' is not unique and can changeBy ensuring the final ordering field is unique and indexed (like id or a created timestamp), we guarantee that the cursor position remains stable: # --- CONSISTENT EXAMPLES ---
class SecureProduct(models.Model):
name = models.CharField(max_length=100)
created_at = models.DateTimeField(auto_now_add=True, db_index=True)
class SecureProductSerializer(serializers.ModelSerializer):
class Meta:
model = SecureProduct
fields = '__all__'
# 1. Consistent PageNumberPagination with Multiple Ordering
class ReliableProductViewSet(viewsets.ModelViewSet):
# Combining a non-unique field with a unique ID ensures consistency
queryset = SecureProduct.objects.all().order_by('name', 'id')
serializer_class = SecureProductSerializer
pagination_class = pagination.PageNumberPagination
# 2. Recommended CursorPagination Usage
class StandardCursorPagination(pagination.CursorPagination):
page_size = 10
ordering = '-created_at' # Unique-ish, indexed, and unchanging
class AccurateProductViewSet(viewsets.ModelViewSet):
queryset = SecureProduct.objects.all()
serializer_class = SecureProductSerializer
pagination_class = StandardCursorPagination |
The ordering recommendations given for the CursorPagination scheme
actually apply to all pagination schemes, an unsuspecting developer that
implements the more common
LimitOffsetPaginationorPageNumberPaginationclasses is unlikely to be aware of the importanceof consistent ordering.
This commit moves the
Details and limitationssection out of theCursorPaginationsection and puts it as the very first subsection ofthe
Paginationpage so that it's one of the first things thatdevelopers see.
Some examples of inconsistencies as well as how to deal with them are
given, and an extra way to change the ordering of a paginated view is
provided.
Fixes #6886
Hopefully this will prevent future developers from scratching their head and wondering why despite a view returning a count of X, going through all the pages yields Y records.