Skip to content

Make DEFAULT_PAGINATION_CLASS None by default. #5170

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.

Sign up for GitHub

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

Merged
carltongibson merged 7 commits into from
Sep 25, 2017
Merged

Make DEFAULT_PAGINATION_CLASS None by default. #5170

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e52d31d
Changes to the paginator defaults and settings
matteius May 23, 2017
a224b6b
DRF-5168 import warnings
matteius May 23, 2017
420bc48
Add a system checks file
matteius Sep 25, 2017
64bc395
more compatible import approach
matteius Sep 25, 2017
f34da6d
missing bactic
matteius Sep 25, 2017
7a0a5c3
revised language and approach to import the system check
matteius Sep 25, 2017
dd8979d
Adjust doc wording
Sep 25, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Changes to the paginator defaults and settings 
Require a default paginator be specified when using the page size
setting.
#5168
  • Loading branch information
@matteius
matteius committed May 23, 2017
commit e52d31d09b1b4f2697ec03384be9e57f05c24b6e
6 changes: 3 additions & 3 deletions docs/api-guide/pagination.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,14 @@ Pagination can be turned off by setting the pagination class to `None`.

## Setting the pagination style

The default pagination style may be set globally, using the `DEFAULT_PAGINATION_CLASS` and `PAGE_SIZE` setting keys. For example, to use the built-in limit/offset pagination, you would do something like this:
The pagination style may be set globally, using the `DEFAULT_PAGINATION_CLASS` and `PAGE_SIZE` setting keys. For example, to use the built-in limit/offset pagination, you would do something like this:

REST_FRAMEWORK = {
'DEFAULT_PAGINATION_CLASS': 'rest_framework.pagination.LimitOffsetPagination',
'PAGE_SIZE': 100
}

Note that you need to set both the pagination class, and the page size that should be used.
Note that you need to set both the pagination class, and the page size that should be used. The `DEFAULT_PAGINATION_CLASS` is set to `None` and disabled by default.

You can also set the pagination class on an individual view by using the `pagination_class` attribute. Typically you'll want to use the same pagination style throughout your API, although you might want to vary individual aspects of the pagination, such as default or maximum page size, on a per-view basis.

Expand Down Expand Up @@ -85,7 +85,7 @@ This pagination style accepts a single number page number in the request query p

#### Setup

To enable the `PageNumberPagination` style globally, use the following configuration, modifying the `PAGE_SIZE` as desired:
To enable the `PageNumberPagination` style globally, use the following configuration, and set the `PAGE_SIZE` as desired:

REST_FRAMEWORK = {
'DEFAULT_PAGINATION_CLASS': 'rest_framework.pagination.PageNumberPagination',
Expand Down
11 changes: 11 additions & 0 deletions rest_framework/pagination.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,17 @@ def invert(x):
class BasePagination(object):
display_page_controls = False

def __init__(self, *args, **kwargs):
# Use of default page size setting requires a default Paginator class
if api_settings.PAGE_SIZE and not api_settings.DEFAULT_PAGINATION_CLASS:
warnings.warn(
"A valid paginator class must be specified with `DEFAULT_PAGINATION_CLASS` "
"when using the `PAGE_SIZE` default pagination setting."
"Defaulting the setting to specifies `PageNumberPagination` "
"is deprecated as pagination is disabled by default.",
DeprecationWarning
)
Copy link
Collaborator

@carltongibson carltongibson Sep 22, 2017

Choose a reason for hiding this comment

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

Is this correct?

By setting 'DEFAULT_PAGINATION_CLASS': None is it not the case that "Defaulting the setting to specifies PageNumberPagination " is simply removed, rather than Deprecated?

Also, could I not set PAGE_SIZE globally and then just enable pagination per-view?

Copy link
Contributor Author

@matteius matteius Sep 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

This check is saying that you cannot have set a default PAGE_SIZE setting without also setting DEFAULT_PAGINATION_CLASS. I believe the language of the second sentence could be improved but its trying to say that defaulting the variable DEFAULT_PAGINATION_CLASS to PageNumberPagination was removed.

The issue here is that the current default in 3.6 is that there is no page size set and PageNumberPagination doesn't have a pagination class defined, so you end up with a case where parts of the code thinks it is paginated but no pagination ever actually occurs . The point of this warning is to prevent users that rely on the current default of PageNumberPagination where it actually works for them because they set PAGE_SIZE and during their upgrade they would realize they need to specify both settings values in their project rather than rely on this former default. When it defaults to None it would disable pagination for those users otherwise.

More information here: #5168

Copy link
Contributor Author

@matteius matteius Sep 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

The PageNumberPagination class defaults to the settings PAGE_SIZE and has page_size_query_param = None so there is no way to change pagination by default without either setting PAGE_SIZE setting or sub-classing PageNumberPagination.

The idea here is that going forward you would want to be explicit in setting a per-class Paginator that makes sense, or to use both defaults to enable pagination across the project space. I recall at Pycon that Tom wanted such a DeprecationWarning to ensure users that upgrade don't go from having pagination on in their project to disabled without any explanation. This concern would be specifically users that set a PAGE_SIZE setting but were relying on the default DEFAULT_PAGINATION_CLASS value.

Copy link
Contributor Author

@matteius matteius Sep 22, 2017

Choose a reason for hiding this comment

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

I think you bring up a good point about users that want to "set PAGE_SIZE globally and then just enable pagination per-view" -- In this case they would specifically be using PageNumberPagination in specific views and have a global PAGE_SIZE set. One option is to sub-class PageNumberPagination and set the page_size setting once on that class, then use that in the per-view instances. Open to refinement, but just stating the history of this PR.

Copy link
Collaborator

@carltongibson carltongibson Sep 22, 2017

Choose a reason for hiding this comment

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

@matteius Thanks for the good replies.

The point of this warning is to prevent users that rely on the current default of PageNumberPagination where it actually works for them because they set PAGE_SIZE and during their upgrade they would realize they need to specify both settings values in their project rather than rely on this former default.

... and ...

...I recall at Pycon that Tom wanted such a DeprecationWarning to ensure users that upgrade don't go from having pagination on in their project to disabled without any explanation.

OK. That's the key.

My thought here is that we should remove the Deprecation warning but add a System Check for the same thing. (It runs once at start up and users can silence this if they really do want just PAGE_SIZE.)

We can then call out the change in the Release Notes and Announcement for 3.7 and the combination of the two would be good enough.

Does that sound OK to you? (Are you happy to write the system check?)

Copy link
Contributor Author

@matteius matteius Sep 22, 2017

Choose a reason for hiding this comment

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

Yes, this sounds like the right direction to head in. I just reviewed the system check docs and I feel comfortable converting this PR to that approach, but where do you suggest is the right file to implement the check method and register it? I was considering doing it in the compat.py file unless you have an alternate suggestion @carltongibson ?
Thanks!

Copy link
Collaborator

@carltongibson carltongibson Sep 23, 2017

Choose a reason for hiding this comment

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

@matteius That sounds fine. (We can always move it later if needed.)

Thanks for the great input! (If you need any help let me know.)

Copy link
Contributor Author

@matteius matteius Sep 25, 2017

Choose a reason for hiding this comment

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

Pull request has been updated to be current.


def paginate_queryset(self, queryset, request, view=None): # pragma: no cover
raise NotImplementedError('paginate_queryset() must be implemented.')

Expand Down
2 changes: 1 addition & 1 deletion rest_framework/settings.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@
'DEFAULT_VERSIONING_CLASS': None,

# Generic view behavior
'DEFAULT_PAGINATION_CLASS': 'rest_framework.pagination.PageNumberPagination',
'DEFAULT_PAGINATION_CLASS': None,
'DEFAULT_FILTER_BACKENDS': (),

# Throttling
Expand Down