-
-
Notifications
You must be signed in to change notification settings - Fork 7.1k
Add MAX_PAGE_SIZE setting #9107
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
base: main
Are you sure you want to change the base?
Changes from all commits
a9d25af
1ada7c7
91695fb
b51141c
6250dc0
0b1a070
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -160,9 +160,6 @@ class PageNumberPagination(BasePagination): | |||||||||||||||||
| http://api.example.org/accounts/?page=4 | ||||||||||||||||||
| http://api.example.org/accounts/?page=4&page_size=100 | ||||||||||||||||||
| """ | ||||||||||||||||||
| # The default page size. | ||||||||||||||||||
| # Defaults to `None`, meaning pagination is disabled. | ||||||||||||||||||
| page_size = api_settings.PAGE_SIZE | ||||||||||||||||||
|
|
||||||||||||||||||
| django_paginator_class = DjangoPaginator | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
@@ -175,16 +172,33 @@ class PageNumberPagination(BasePagination): | |||||||||||||||||
| page_size_query_param = None | ||||||||||||||||||
| page_size_query_description = _('Number of results to return per page.') | ||||||||||||||||||
|
|
||||||||||||||||||
| # Set to an integer to limit the maximum page size the client may request. | ||||||||||||||||||
| # Only relevant if 'page_size_query_param' has also been set. | ||||||||||||||||||
| max_page_size = None | ||||||||||||||||||
|
|
||||||||||||||||||
| last_page_strings = ('last',) | ||||||||||||||||||
|
|
||||||||||||||||||
| template = 'rest_framework/pagination/numbers.html' | ||||||||||||||||||
|
|
||||||||||||||||||
| invalid_page_message = _('Invalid page.') | ||||||||||||||||||
|
|
||||||||||||||||||
| @property | ||||||||||||||||||
| def page_size(self) -> int: | ||||||||||||||||||
|
||||||||||||||||||
| """Get default page size. | ||||||||||||||||||
|
|
||||||||||||||||||
| Defaults to `None`, meaning pagination is disabled. | ||||||||||||||||||
|
|
||||||||||||||||||
| """ | ||||||||||||||||||
|
Comment on lines
+182
to
+187
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There shoud probably be a comment explaining why this i a property to avoid removing it in a future refactor? |
||||||||||||||||||
| return api_settings.PAGE_SIZE | ||||||||||||||||||
|
|
||||||||||||||||||
| @property | ||||||||||||||||||
| def max_page_size(self) -> int: | ||||||||||||||||||
|
||||||||||||||||||
| """Limit page size. | ||||||||||||||||||
|
|
||||||||||||||||||
| Set to an integer to limit the maximum page size the client may request. | ||||||||||||||||||
| Only relevant if 'page_size_query_param' has also been set. | ||||||||||||||||||
| Defaults to `None`, meaning page size is unlimited. | ||||||||||||||||||
| It's recommended that you would set a limit to avoid api abuse. | ||||||||||||||||||
|
||||||||||||||||||
| It's recommended that you would set a limit to avoid api abuse. | |
| It's recommended that you set a limit to avoid API abuse. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we avoid changing the style here?
max_limit = api_settings.MAX_PAGE_SIZEPerfer minimal impact footprints for PRs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's related to #8993 and #9107 (comment). Without it we can't properly test it, since this setting will be set during init.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hrm... not sure? If you'd like to take that approach then I'd probably suggest handling #8993 separately, as a precursor to this.
Unclear to me why that'd be different to attributes we use in other cases.
Copilot
AI
Apr 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same typing concern as elsewhere: api_settings.MAX_PAGE_SIZE (and PAGE_SIZE) default to None, so annotating these as int is inaccurate. Adjust the annotations to allow None or omit them for consistency with the rest of the module.
Copilot
AI
Apr 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LimitOffsetPagination uses limit_query_param, not page_size_query_param, so this docstring is misleading. Also the same grammar/casing issue applies (“API abuse”). Update the text to refer to limit_query_param (and/or max_limit) and fix the wording.
| Set to an integer to limit the maximum page size the client may request. | |
| Only relevant if 'page_size_query_param' has also been set. | |
| Defaults to `None`, meaning page size is unlimited. | |
| It's recommended that you would set a limit to avoid api abuse. | |
| Set to an integer to limit the maximum number of results the client may | |
| request via the ``limit_query_param``. | |
| Defaults to ``None``, meaning the limit is unrestricted. | |
| It is recommended that you set a limit to avoid API abuse. |
Copilot
AI
Apr 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MAX_PAGE_SIZE is now wired into LimitOffsetPagination via max_limit (and also into CursorPagination/PageNumberPagination). There are new integration tests for PageNumberPagination settings, but no corresponding tests that MAX_PAGE_SIZE actually caps limit for LimitOffsetPagination (and/or caps cursor page size when enabled). Adding a couple of targeted tests would help prevent regressions across pagination styles.
Copilot
AI
Apr 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These settings-backed properties can return None (MAX_PAGE_SIZE defaults to None), but the return annotation is int. Consider changing to Optional[int]/int | None (or removing the annotation) to avoid misleading type information.
Copilot
AI
Apr 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same docstring grammar/casing issue as earlier (“It's recommended that you would…”, “api” → “API”). Please rephrase for correct English and consistent capitalization.
| It's recommended that you would set a limit to avoid api abuse. | |
| It is recommended to set a limit to avoid API abuse. |
Copilot
AI
Apr 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CursorPagination.page_size is now a read-only @Property, but paginate_queryset() assigns to self.page_size. That assignment will raise AttributeError at runtime. Consider either keeping page_size as a regular attribute, or adding a setter/backing attribute so paginate_queryset can cache the computed per-request page size safely.
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -65,6 +65,7 @@ | |||||
|
|
||||||
| # Pagination | ||||||
| 'PAGE_SIZE': None, | ||||||
| "MAX_PAGE_SIZE": None, | ||||||
|
||||||
| "MAX_PAGE_SIZE": None, | |
| 'MAX_PAGE_SIZE': None, |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,7 @@ | ||
| import pytest | ||
| from django.core.paginator import Paginator as DjangoPaginator | ||
| from django.db import models | ||
| from django.test import TestCase | ||
| from django.test import TestCase, override_settings | ||
|
|
||
| from rest_framework import ( | ||
| exceptions, filters, generics, pagination, serializers, status | ||
|
|
@@ -135,6 +135,77 @@ def test_404_not_found_for_invalid_page(self): | |
| } | ||
|
|
||
|
|
||
| class TestPaginationSettingsIntegration: | ||
| """ | ||
| Integration tests for pagination settings. | ||
| """ | ||
|
|
||
| def setup_method(self): | ||
| class PassThroughSerializer(serializers.BaseSerializer): | ||
| def to_representation(self, item): | ||
| return item | ||
|
|
||
| class EvenItemsOnly(filters.BaseFilterBackend): | ||
| def filter_queryset(self, request, queryset, view): | ||
| return [item for item in queryset if item % 2 == 0] | ||
|
|
||
| class BasicPagination(pagination.PageNumberPagination): | ||
| page_size_query_param = 'page_size' | ||
|
|
||
| self.view = generics.ListAPIView.as_view( | ||
| serializer_class=PassThroughSerializer, | ||
| queryset=range(1, 101), | ||
| filter_backends=[EvenItemsOnly], | ||
| pagination_class=BasicPagination | ||
| ) | ||
|
|
||
| @override_settings( | ||
| REST_FRAMEWORK={ | ||
| "MAX_PAGE_SIZE": 20, | ||
| "PAGE_SIZE": 5, | ||
| } | ||
| ) | ||
|
Comment on lines
+162
to
+167
|
||
| def test_setting_page_size_over_maximum(self): | ||
| """ | ||
| When page_size parameter exceeds maximum allowable, | ||
| then it should be capped to the maximum. | ||
| """ | ||
| request = factory.get('/', {'page_size': 1000}) | ||
| response = self.view(request) | ||
| assert response.status_code == status.HTTP_200_OK | ||
| assert len(response.data["results"]) == 20, response.data | ||
| assert response.data == { | ||
| 'results': [ | ||
| 2, 4, 6, 8, 10, 12, 14, 16, 18, 20, | ||
| 22, 24, 26, 28, 30, 32, 34, 36, 38, 40 | ||
| ], | ||
| 'previous': None, | ||
| 'next': 'http://testserver/?page=2&page_size=1000', | ||
| 'count': 50 | ||
| } | ||
|
|
||
| @override_settings( | ||
| REST_FRAMEWORK={ | ||
| "MAX_PAGE_SIZE": 20, | ||
| "PAGE_SIZE": 5, | ||
| } | ||
| ) | ||
| def test_setting_page_size_to_zero(self): | ||
| """ | ||
| When page_size parameter is invalid it should return to the default. | ||
| """ | ||
| request = factory.get('/', {'page_size': 0}) | ||
| response = self.view(request) | ||
| assert response.status_code == status.HTTP_200_OK | ||
| assert len(response.data["results"]) == 5, response.data | ||
| assert response.data == { | ||
| 'results': [2, 4, 6, 8, 10], | ||
| 'previous': None, | ||
| 'next': 'http://testserver/?page=2&page_size=0', | ||
| 'count': 50 | ||
| } | ||
|
|
||
|
|
||
| class TestPaginationDisabledIntegration: | ||
| """ | ||
| Integration tests for disabled pagination. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This note is a bit too strong/inaccurate: you don’t always need to set both PAGE_SIZE and MAX_PAGE_SIZE for pagination to work (e.g., LimitOffsetPagination can rely on the client-provided
limit, and MAX_PAGE_SIZE only matters when a client-controllable size param is enabled). Consider rewording to clarify which settings are required vs optional depending on the pagination class.