Make DEFAULT_PAGINATION_CLASS None by default.

[matteius]

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.

[carltongibson]

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?

[carltongibson]

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?

[matteius]

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

[matteius]

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.

[matteius]

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.

[carltongibson]

@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?)

[matteius]

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!

[carltongibson]

@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.)

[matteius]

Pull request has been updated to be current.

[matteius]

@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.

[carltongibson]

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! 👍

[carltongibson]

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.

[carltongibson]

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

[matteius]

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

[carltongibson]

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

[novelview9]

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]

@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]

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