Add a setting for max page size for global pagination. Our team discovered that by default, page size is unlimited which could lead to API abuse and the only way to fix it -> it's to subclass pagination class(which is a working solution, but not convenient). After investigating issues I found relatedissue andcomment.

auvipy self-requested a review

September 14, 2023 14:11

auvipy suggested changes

Sep 14, 2023

View reviewed changes

Copy link

Member

auvipy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I think this deserve a test case,, but as it is adding/exposing new API surface to the framework, we might need to pass the design decision step first

Copy link

Member

auvipy commentedSep 14, 2023•
edited
Loading

OK I saw the comment now#6185 (comment) it might be possible in class level but not globally at the moment.

auvipy added this to the3.15 milestone

Sep 14, 2023

Copy link

ContributorAuthor

TheSuperiorStanislav commentedSep 15, 2023

@auvipy I added tests for system checks, but didn't managed to find and add tests for settings with API requests like intest_pagination.py, and it's kind hard to add since pagination settings are defined as class attrs.

TheSuperiorStanislav requested a review fromauvipy

September 15, 2023 04:16

auvipy requested a review froma team

September 19, 2023 07:44

auvipy mentioned this pull request

Oct 2, 2023

Allow REST_FRAMEWORK.PAGE_SIZE to be overriden in settings#8993

Closed

Copy link

Member

auvipy commentedOct 2, 2023

we got a PR related to this area#8993. would be helpful if you review and share your views on the PR.

christophehenry reviewed

Oct 9, 2023

View reviewed changes

rest_framework/pagination.py OutdatedShow resolvedHide resolved

christophehenry reviewed

Oct 9, 2023

View reviewed changes

rest_framework/pagination.py OutdatedShow resolvedHide resolved

christophehenry reviewed

Oct 9, 2023

View reviewed changes

rest_framework/pagination.py OutdatedShow resolvedHide resolved

Copy link

ankitchhatbar commentedDec 22, 2023•
edited
Loading

@auvipy @christophehenry @TheSuperiorStanislav If you don't mind, I can take this up and implement the changes requested

TheSuperiorStanislavand others added4 commits

August 21, 2024 08:16

Add MAX_PAGE_SIZE setting

a9d25af

Update check for pagination settings, update test

1ada7c7

Fix setting typo

91695fb

Co-authored-by: Roman Gorbil <roman.gorbil@saritasa.com>

Move page settings to property

b51141c

TheSuperiorStanislav force-pushed theadd-max-page-setting-for-pagination branch from456a1fe tob51141cCompare

August 21, 2024 02:02

TheSuperiorStanislav requested a review fromchristophehenry

August 21, 2024 02:06

Copy link

ContributorAuthor

TheSuperiorStanislav commentedAug 21, 2024•
edited
Loading

@auvipy @christophehenry Could you please take a look? Sorry for taking that long, btw 🥲

christophehenry reviewed

Aug 21, 2024

View reviewed changes

rest_framework/pagination.py

Comment on lines +191 to +196

		def page_size(self) -> int:
		"""Get default page size.

		Defaults to `None`, meaning pagination is disabled.

		"""

Copy link

christophehenryAug 21, 2024

Choose a reason for hiding this comment

The 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?

christophehenry approved these changes

Aug 21, 2024

View reviewed changes

Copy link

christophehenry commentedAug 21, 2024•
edited
Loading

Looks good to me. Thank you.

Merge branch 'encode:master' into add-max-page-setting-for-pagination

6250dc0

Copy link

ContributorAuthor

TheSuperiorStanislav commentedSep 19, 2024

@auvipy Could you take a look, please? This pr has been here for a while)

tomchristie reviewed

Sep 19, 2024

View reviewed changes

rest_framework/pagination.py

		limit_query_param = 'limit'
		limit_query_description = _('Number of results to return per page.')
		offset_query_param = 'offset'
		offset_query_description = _('The initial index from which to return the results.')
		max_limit = None

Copy link

Member

tomchristieSep 19, 2024

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_SIZE

Perfer minimal impact footprints for PRs.

Copy link

ContributorAuthor

TheSuperiorStanislavSep 20, 2024•
edited
Loading

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.

Copy link

Member

tomchristieSep 20, 2024•
edited
Loading

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 attributeswe use in other cases.

TheSuperiorStanislav requested a review fromtomchristie

September 20, 2024 06:39

Copy link

stalebot commentedApr 26, 2025

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.