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

Make DEFAULT_PAGINATION_CLASS None by default. #5170

Merged
merged 7 commits into from Sep 25, 2017

Conversation

matteius
Copy link
Contributor

@matteius matteius commented May 23, 2017

Description

To Address: #5168

This is followup work to a roll back change I made yesterday that fixed schema generation when pagination was enabled. That change was to restore some behavior that was considered a regression in 3.5.6 and there was followup to do to make more sane default settings for pagination.

Prior to this change there was a default pagination class with no global page size defined which resulted in a disabled pagination configuration. This work is to accept that the default is pagination is disabled, and to help users upgrade to 3.7 version that includes a DeprecationWarning and a documentation change.

matteius added 2 commits May 23, 2017
Require a default paginator be specified when using the page size
setting.
encode#5168
missed this in last commit
@tomchristie tomchristie added this to the 3.7.0 Release milestone May 29, 2017
@tomchristie tomchristie changed the title DRF-5168 More reasonable defaults for global pagination settings. More reasonable defaults for global pagination settings. May 29, 2017
Copy link
Collaborator

@carltongibson carltongibson left a comment

I'd be happy with this. Just the comment about the warning...

Given the 3.7, can we just put it in the release notes and be done with it?

"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

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

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.

matteius added 2 commits Sep 25, 2017
Add a check for pagination settings for the 3.7 upgrade cycle.
@matteius
Copy link
Contributor Author

matteius commented Sep 25, 2017

@carltongibson Please see the current form of this PR -- I made the change to use system checks and verified it working in a test project. It didn't work to just add it to compat.py because nothing imported the check by default. I've added a checks.py where we can register current system checks and I import that from init.py. The language has been revised to be more helpful. I saw this warning when I ran runserver and the shell in my test project:

WARNINGS:
?: You have specified a default PAGE_SIZEpagination rest_framework setting,without specifying also aDEFAULT_PAGINATION_CLASS. HINT: The prior version of rest_framework defaulted this setting to PageNumberPaginationhowever pagination defaults to disabled now. Consider specifyingDEFAULT_PAGINATION_CLASS` explicitly for your project, unless you specify individual pagination_class values on specific view classes.

Copy link
Collaborator

@carltongibson carltongibson left a comment

OK This is great.

  • If we can use an AppConfig to import the check, as per docs/comment, that will be perfect.
  • I'm not 100% sure on the wording of the warning, but we can tweak that easily. (I don't want to block this on that now.)

Thanks for the super effort! 👍

@@ -6,6 +6,7 @@
| |\ \| |___/\__/ / | | | | | | | (_| | | | | | | __/\ V V / (_) | | | <
\_| \_\____/\____/ \_/ |_| |_| \__,_|_| |_| |_|\___| \_/\_/ \___/|_| |_|\_|
"""
from .checks import * # NOQA
Copy link
Collaborator

@carltongibson carltongibson Sep 25, 2017

Choose a reason for hiding this comment

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

The docs on Registering and labeling checks suggest using an AppConfig.ready(). I think we should follow that.

Docs on AppConfig configuration

  1. Add the AppConfig class in rest_framwork/apps.py.
  2. Import check in ready, with comment explaining purpose.
  3. Set default_app_config here.

"You have specified a default `PAGE_SIZE` pagination rest_framework setting,"
"without specifying also a `DEFAULT_PAGINATION_CLASS`.",
hint="The prior version of rest_framework defaulted this setting to "
"`PageNumberPagination` however pagination defaults to disabled now. "
Copy link
Collaborator

@carltongibson carltongibson Sep 25, 2017

Choose a reason for hiding this comment

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

"The default for DEFAULT_PAGINATION_CLASS is None. (In previous versions this was PageNumberPagination)"

@matteius
Copy link
Contributor Author

matteius commented Sep 25, 2017

@carltongibson Ok I've pushed another set of changes. Let me know if anything else I can do to wrap this one up. Thanks!

Copy link
Collaborator

@carltongibson carltongibson left a comment

@matteius Great work thank you! Let's have it. 💃🏼

carltongibson added a commit that referenced this pull request Sep 25, 2017
@carltongibson carltongibson changed the title More reasonable defaults for global pagination settings. Make DEFAULT_PAGINATION_CLASS None by default. Sep 25, 2017
@carltongibson carltongibson merged commit 107e8b3 into encode:master Sep 25, 2017
1 check passed
@matteius matteius deleted the DRF-5168 branch Sep 25, 2017
carltongibson added a commit that referenced this pull request Sep 26, 2017
carltongibson added a commit that referenced this pull request Sep 27, 2017
carltongibson added a commit that referenced this pull request Sep 28, 2017
carltongibson added a commit that referenced this pull request Oct 5, 2017
carltongibson added a commit that referenced this pull request Oct 5, 2017
carltongibson pushed a commit that referenced this pull request Oct 6, 2017
* Set version number for 3.7.0 release

* Rename release notes section

Moved issue links to top for easier access.
(Can move back later)

* Add release note for #5273

* Add release note for #5440

* Add release note for #5265

Strict JSON handling

* Add release note for #5250

* Add release notes for #5170

* Add release notes for #5443

* Add release notes for #5448

* Add release notes for #5452

* Add release not for #5342

* Add release notes for 5454

* Add release notes for #5058 & #5457

Remove Django 1.8 & 1.9 from README and setup.py

* Release notes for merged 3.6.5 milestone tickets

Tickets migrated to 3.7.0 milestone.

* Add release notes for #5469

* Add release notes from AM 2ndOct

* Add final changes to the release notes.

* Add date and milestone link

Move issue links back to bottom.

* Update translations from transifex

* Begin releae anouncement

* Add release note for #5482

* 3.7 release announcement & related docs.
@novelview9
Copy link

novelview9 commented Oct 23, 2017

check.py in 107e8b3
I think this error message has something wrong.
If someone doesn't want to use pagination as a default('DEFAULT_PAGINATION_CLASS': None),
but somecase when need to declare pagination manully on the viewclass,
someone can be wanted to set default PAGE_SIZE for manual pagination.

@carltongibson
Copy link
Collaborator

carltongibson commented Oct 23, 2017

@novelview9 See the discussion following here #5170 (comment)

tl;dr: Yes, totally valid use-case. Add to SILENCED_SYSTEM_CHECKS setting as per System Checks docs if you've seen the warning and it doesn't apply to you.

@novelview9
Copy link

novelview9 commented Oct 23, 2017

Oh, I didn't read comments carefully before, sorry -:)
In that purpose,
what about to do like below

    # Use of default page size setting requires a default Paginator class
    from rest_framework.settings import api_settings
    if api_settings.PAGE_SIZE and ("DEFAULT_PAGINATION_CLASS" not in api_settings.user_settings):
        errors.append(
            Warning(
                "You have specified a default PAGE_SIZE pagination rest_framework setting,"
                "without specifying also a DEFAULT_PAGINATION_CLASS.",
                hint="The default for DEFAULT_PAGINATION_CLASS is None. "
                     "In previous versions this was PageNumberPagination",
                    "PAGE_SIZE needs specifying a DEFAULT_PAGINATION_CLASS Even though the value is None,"
            )
        )
    return errors

it makes sense and more explicit

novelview9 added a commit to novelview9/django-rest-framework that referenced this pull request Oct 23, 2017
novelview9 added a commit to novelview9/django-rest-framework that referenced this pull request Oct 23, 2017
@novelview9 novelview9 mentioned this pull request Oct 23, 2017
carltongibson added a commit to carltongibson/django-rest-framework that referenced this pull request Oct 23, 2017
* Add uid=501(carlton) gid=20(staff) groups=20(staff),250(com.apple.access_backup),401(com.apple.sharepoint.group.1),12(everyone),61(localaccounts),79(_appserverusr),80(admin),81(_appserveradm),98(_lpadmin),402(com.apple.sharepoint.group.2),33(_appstore),100(_lpoperator),204(_developer),395(com.apple.access_ftp),398(com.apple.access_screensharing),399(com.apple.access_ssh) to allow silencing.
* Expand  to clarify.

Ref encode#5170 Closes encode#5523
carltongibson added a commit to carltongibson/django-rest-framework that referenced this pull request Oct 23, 2017
* Add `id` to allow silencing.
* Expand `hint` to clarify.

Ref encode#5170 Closes encode#5523
carltongibson added a commit that referenced this pull request Oct 23, 2017
* Add `id` to allow silencing.
* Expand `hint` to clarify.

Ref #5170 Closes #5523
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

4 participants