Use DRF 3.1 `pagination_class` (#3559)

* Use DRF 3.1 `pagination_class`

* Update pre-commit and styles

This style corresponds to PR #3559

* Lint

* Pagination tests

.pre-commit-config.yaml

@@ -2,7 +2,7 @@ exclude: '^$'
 fail_fast: false
 repos:
 - repo: git@github.com:pre-commit/pre-commit-hooks
-  sha: v1.1.1
+  sha: v1.2.0
   hooks:
     - id: autopep8-wrapper
     - id: check-added-large-files
@@ -30,7 +30,7 @@ repos:
     - id: trailing-whitespace
 
 - repo: git@github.com:pre-commit/mirrors-yapf.git
-  sha: v0.20.0
+  sha: v0.20.1
   hooks:
     - id: yapf
       exclude: 'migrations|settings|scripts'
@@ -56,9 +56,10 @@ repos:
       args: ['--in-place', '--remove-all-unused-imports', '--remove-unused-variable']
 
 - repo: git://github.com/guykisel/prospector-mirror
-  sha: 'b27f281eb9398fc8504415d7fbdabf119ea8c5e1'
+  sha: b27f281eb9398fc8504415d7fbdabf119ea8c5e1
   hooks:
     - id: prospector
       # https://github.com/pre-commit/pre-commit/issues/178
       language: system
+      files: '\.py$'
       args: ['--profile=prospector']

readthedocs/restapi/utils.py

@@ -1,9 +1,14 @@
+# -*- coding: utf-8 -*-
 """Utility functions that are used by both views and celery tasks."""
 
-from __future__ import absolute_import
+from __future__ import (
+    absolute_import, division, print_function, unicode_literals)
+
 import hashlib
 import logging
 
+from rest_framework.pagination import PageNumberPagination
+
 from readthedocs.builds.constants import NON_REPOSITORY_VERSIONS
 from readthedocs.builds.models import Version
 from readthedocs.search.indexes import PageIndex, ProjectIndex, SectionIndex
@@ -31,14 +36,17 @@ def sync_versions(project, versions, type):  # pylint: disable=redefined-builtin
             else:
                 # Update slug with new identifier
                 Version.objects.filter(
-                    project=project, verbose_name=version_name
-                ).update(
-                    identifier=version_id,
-                    type=type,
-                    machine=False,
+                    project=project, verbose_name=version_name).update(
+                        identifier=version_id,
+                        type=type,
+                        machine=False,
+                    )  # noqa
+
+                log.info(
+                    '(Sync Versions) Updated Version: [%s=%s] ',
+                    version['verbose_name'],
+                    version['identifier'],
                 )
-                log.info("(Sync Versions) Updated Version: [%s=%s] ",
-                         version['verbose_name'], version['identifier'])
         else:
             # New Version
             created_version = Version.objects.create(
@@ -49,7 +57,7 @@ def sync_versions(project, versions, type):  # pylint: disable=redefined-builtin
             )
             added.add(created_version.slug)
     if added:
-        log.info("(Sync Versions) Added Versions: [%s] ", ' '.join(added))
+        log.info('(Sync Versions) Added Versions: [%s] ', ' '.join(added))
     return added
 
 
@@ -70,14 +78,15 @@ def delete_versions(project, version_data):
 
     if to_delete_qs.count():
         ret_val = {obj.slug for obj in to_delete_qs}
-        log.info("(Sync Versions) Deleted Versions: [%s]", ' '.join(ret_val))
+        log.info('(Sync Versions) Deleted Versions: [%s]', ' '.join(ret_val))
         to_delete_qs.delete()
         return ret_val
     return set()
 
 
-def index_search_request(version, page_list, commit, project_scale, page_scale,
-                         section=True, delete=True):
+def index_search_request(
+        version, page_list, commit, project_scale, page_scale, section=True,
+        delete=True):
     """
     Update search indexes with build output JSON.
 
@@ -89,21 +98,25 @@ def index_search_request(version, page_list, commit, project_scale, page_scale,
     project = version.project
 
     log_msg = ' '.join([page['path'] for page in page_list])
-    log.info("Updating search index: project=%s pages=[%s]",
-             project.slug, log_msg)
+    log.info(
+        'Updating search index: project=%s pages=[%s]',
+        project.slug,
+        log_msg,
+    )
 
     project_obj = ProjectIndex()
-    project_obj.index_document(data={
-        'id': project.pk,
-        'name': project.name,
-        'slug': project.slug,
-        'description': project.description,
-        'lang': project.language,
-        'author': [user.username for user in project.users.all()],
-        'url': project.get_absolute_url(),
-        'tags': None,
-        'weight': project_scale,
-    })
+    project_obj.index_document(
+        data={
+            'id': project.pk,
+            'name': project.name,
+            'slug': project.slug,
+            'description': project.description,
+            'lang': project.language,
+            'author': [user.username for user in project.users.all()],
+            'url': project.get_absolute_url(),
+            'tags': None,
+            'weight': project_scale,
+        })
 
     page_obj = PageIndex()
     section_obj = SectionIndex()
@@ -112,7 +125,7 @@ def index_search_request(version, page_list, commit, project_scale, page_scale,
     routes = [project.slug]
     routes.extend([p.parent.slug for p in project.superprojects.all()])
     for page in page_list:
-        log.debug("Indexing page: %s:%s", project.slug, page['path'])
+        log.debug('Indexing page: %s:%s', project.slug, page['path'])
         to_hash = '-'.join([project.slug, version.slug, page['path']])
         page_id = hashlib.md5(to_hash.encode('utf-8')).hexdigest()
         index_list.append({
@@ -129,8 +142,12 @@ def index_search_request(version, page_list, commit, project_scale, page_scale,
         })
         if section:
             for sect in page['sections']:
-                id_to_hash = '-'.join([project.slug, version.slug,
-                                       page['path'], sect['id']])
+                id_to_hash = '-'.join([
+                    project.slug,
+                    version.slug,
+                    page['path'],
+                    sect['id'],
+                ])
                 section_index_list.append({
                     'id': (hashlib.md5(id_to_hash.encode('utf-8')).hexdigest()),
                     'project': project.slug,
@@ -142,28 +159,52 @@ def index_search_request(version, page_list, commit, project_scale, page_scale,
                     'weight': page_scale,
                 })
             for route in routes:
-                section_obj.bulk_index(section_index_list, parent=page_id,
-                                       routing=route)
+                section_obj.bulk_index(
+                    section_index_list,
+                    parent=page_id,
+                    routing=route,
+                )
 
     for route in routes:
         page_obj.bulk_index(index_list, parent=project.slug, routing=route)
 
     if delete:
-        log.info("Deleting files not in commit: %s", commit)
+        log.info('Deleting files not in commit: %s', commit)
         # TODO: AK Make sure this works
         delete_query = {
-            "query": {
-                "bool": {
-                    "must": [
-                        {"term": {"project": project.slug, }},
-                        {"term": {"version": version.slug, }},
+            'query': {
+                'bool': {
+                    'must': [
+                        {
+                            'term': {
+                                'project': project.slug,
+                            },
+                        },
+                        {
+                            'term': {
+                                'version': version.slug,
+                            },
+                        },
                     ],
-                    "must_not": {
-                        "term": {
-                            "commit": commit
-                        }
-                    }
-                }
-            }
+                    'must_not': {
+                        'term': {
+                            'commit': commit,
+                        },
+                    },
+                },
+            },
         }
         page_obj.delete_document(body=delete_query)
+
+
+class RemoteOrganizationPagination(PageNumberPagination):
+    page_size = 25
+
+
+class RemoteProjectPagination(PageNumberPagination):
+    page_size = 15
+
+
+class ProjectPagination(PageNumberPagination):
+    page_size = 100
+    max_page_size = 1000

readthedocs/restapi/views/model_views.py

@@ -1,33 +1,34 @@
+# -*- coding: utf-8 -*-
 """Endpoints for listing Projects, Versions, Builds, etc."""
 
-from __future__ import absolute_import
+from __future__ import (
+    absolute_import, division, print_function, unicode_literals)
+
 import logging
 
 from django.shortcuts import get_object_or_404
-from rest_framework import decorators, permissions, viewsets, status
+from rest_framework import decorators, permissions, status, viewsets
 from rest_framework.decorators import detail_route
 from rest_framework.renderers import JSONRenderer
 from rest_framework.response import Response
 
-from readthedocs.builds.constants import BRANCH
-from readthedocs.builds.constants import TAG
+from readthedocs.builds.constants import BRANCH, TAG
 from readthedocs.builds.models import Build, BuildCommandResult, Version
 from readthedocs.core.utils import trigger_build
 from readthedocs.core.utils.extend import SettingsOverrideObject
-from readthedocs.oauth.services import GitHubService, registry
 from readthedocs.oauth.models import RemoteOrganization, RemoteRepository
-from readthedocs.projects.models import Project, EmailHook, Domain
+from readthedocs.oauth.services import GitHubService, registry
+from readthedocs.projects.models import Domain, EmailHook, Project
 from readthedocs.projects.version_handling import determine_stable_version
 
-from ..permissions import (APIPermission, APIRestrictedPermission,
-                           RelatedProjectIsOwner, IsOwner)
-from ..serializers import (BuildSerializer, BuildAdminSerializer,
-                           BuildCommandSerializer,
-                           ProjectSerializer, ProjectAdminSerializer,
-                           VersionSerializer, VersionAdminSerializer,
-                           DomainSerializer, RemoteOrganizationSerializer,
-                           RemoteRepositorySerializer)
 from .. import utils as api_utils
+from ..permissions import (
+    APIPermission, APIRestrictedPermission, IsOwner, RelatedProjectIsOwner)
+from ..serializers import (
+    BuildAdminSerializer, BuildCommandSerializer, BuildSerializer,
+    DomainSerializer, ProjectAdminSerializer, ProjectSerializer,
+    RemoteOrganizationSerializer, RemoteRepositorySerializer,
+    VersionAdminSerializer, VersionSerializer)
 
 log = logging.getLogger(__name__)
 
@@ -44,7 +45,8 @@ class UserSelectViewSet(viewsets.ModelViewSet):
 
     def get_serializer_class(self):
         try:
-            if self.request.user.is_staff and self.admin_serializer_class is not None:
+            if (self.request.user.is_staff and
+                    self.admin_serializer_class is not None):
                 return self.admin_serializer_class
         except AttributeError:
             pass
@@ -57,31 +59,34 @@ class UserSelectViewSet(viewsets.ModelViewSet):
 
 class ProjectViewSet(UserSelectViewSet):
 
-    """List, filter, etc. Projects."""
+    """List, filter, etc, Projects."""
 
     permission_classes = [APIPermission]
     renderer_classes = (JSONRenderer,)
     serializer_class = ProjectSerializer
     admin_serializer_class = ProjectAdminSerializer
     model = Project
-    paginate_by = 100
-    paginate_by_param = 'page_size'
-    max_paginate_by = 1000
+    pagination_class = api_utils.ProjectPagination
 
     @decorators.detail_route()
     def valid_versions(self, request, **kwargs):
         """Maintain state of versions that are wanted."""
         project = get_object_or_404(
             Project.objects.api(request.user), pk=kwargs['pk'])
-        if not project.num_major or not project.num_minor or not project.num_point:
+        if (not project.num_major or not project.num_minor or
+                not project.num_point):
             return Response(
-                {'error': 'Project does not support point version control'},
-                status=status.HTTP_400_BAD_REQUEST)
+                {
+                    'error': 'Project does not support point version control',
+                },
+                status=status.HTTP_400_BAD_REQUEST,
+            )
         version_strings = project.supported_versions()
         # Disable making old versions inactive for now.
         # project.versions.exclude(verbose_name__in=version_strings).update(active=False)
-        project.versions.filter(
-            verbose_name__in=version_strings).update(active=True)
+        project.versions.filter(verbose_name__in=version_strings).update(
+            active=True,
+        )
         return Response({
             'flat': version_strings,
         })
@@ -90,7 +95,7 @@ class ProjectViewSet(UserSelectViewSet):
     def translations(self, *_, **__):
         translations = self.get_object().translations.all()
         return Response({
-            'translations': ProjectSerializer(translations, many=True).data
+            'translations': ProjectSerializer(translations, many=True).data,
         })
 
     @detail_route()
@@ -100,7 +105,7 @@ class ProjectViewSet(UserSelectViewSet):
         rels = project.subprojects.all()
         children = [rel.child for rel in rels]
         return Response({
-            'subprojects': ProjectSerializer(children, many=True).data
+            'subprojects': ProjectSerializer(children, many=True).data,
         })
 
     @detail_route()
@@ -109,7 +114,7 @@ class ProjectViewSet(UserSelectViewSet):
             Project.objects.api(request.user), pk=kwargs['pk'])
         versions = project.versions.filter(active=True)
         return Response({
-            'versions': VersionSerializer(versions, many=True).data
+            'versions': VersionSerializer(versions, many=True).data,
         })
 
     @decorators.detail_route(permission_classes=[permissions.IsAdminUser])
@@ -118,7 +123,7 @@ class ProjectViewSet(UserSelectViewSet):
             Project.objects.api(request.user), pk=kwargs['pk'])
         token = GitHubService.get_token_for_project(project, force_local=True)
         return Response({
-            'token': token
+            'token': token,
         })
 
     @decorators.detail_route()
@@ -126,16 +131,18 @@ class ProjectViewSet(UserSelectViewSet):
         project = get_object_or_404(
             Project.objects.api(request.user), pk=kwargs['pk'])
         return Response({
-            'url': project.get_docs_url()
+            'url': project.get_docs_url(),
         })
 
-    @decorators.detail_route(permission_classes=[permissions.IsAdminUser], methods=['post'])
+    @decorators.detail_route(
+        permission_classes=[permissions.IsAdminUser], methods=['post'])
     def sync_versions(self, request, **kwargs):  # noqa: D205
         """
-        Sync the version data in the repo (on the build server) with what we
-        have in the database.
+        Sync the version data in the repo (on the build server).
 
-        Returns the identifiers for the versions that have been deleted.
+        Version data in the repo is synced with what we have in the database.
+
+        :returns: the identifiers for the versions that have been deleted.
         """
         project = get_object_or_404(
             Project.objects.api(request.user), pk=kwargs['pk'])
@@ -162,22 +169,27 @@ class ProjectViewSet(UserSelectViewSet):
                 added_versions.update(ret_set)
             deleted_versions = api_utils.delete_versions(project, data)
         except Exception as e:
-            log.exception("Sync Versions Error: %s", e.message)
-            return Response({'error': e.message}, status=status.HTTP_400_BAD_REQUEST)
+            log.exception('Sync Versions Error: %s', e.message)
+            return Response(
+                {
+                    'error': e.message,
+                },
+                status=status.HTTP_400_BAD_REQUEST,
+            )
 
         promoted_version = project.update_stable_version()
         if promoted_version:
             new_stable = project.get_stable_version()
             log.info(
-                "Triggering new stable build: {project}:{version}".format(
+                'Triggering new stable build: {project}:{version}'.format(
                     project=project.slug,
-                    version=new_stable.identifier))
+                    version=new_stable.identifier,
+                ))
             trigger_build(project=project, version=new_stable)
 
             # Marking the tag that is considered the new stable version as
             # active and building it if it was just added.
-            if (
-                    activate_new_stable and
+            if (activate_new_stable and
                     promoted_version.slug in added_versions):
                 promoted_version.active = True
                 promoted_version.save()
@@ -241,12 +253,14 @@ class RemoteOrganizationViewSet(viewsets.ReadOnlyModelViewSet):
     renderer_classes = (JSONRenderer,)
     serializer_class = RemoteOrganizationSerializer
     model = RemoteOrganization
-    paginate_by = 25
+    pagination_class = api_utils.RemoteOrganizationPagination
 
     def get_queryset(self):
-        return (self.model.objects.api(self.request.user)
-                .filter(account__provider__in=[service.adapter.provider_id
-                                               for service in registry]))
+        return (
+            self.model.objects.api(self.request.user).filter(
+                account__provider__in=[
+                    service.adapter.provider_id for service in registry
+                ]))
 
 
 class RemoteRepositoryViewSet(viewsets.ReadOnlyModelViewSet):
@@ -254,15 +268,15 @@ class RemoteRepositoryViewSet(viewsets.ReadOnlyModelViewSet):
     renderer_classes = (JSONRenderer,)
     serializer_class = RemoteRepositorySerializer
     model = RemoteRepository
+    pagination_class = api_utils.RemoteProjectPagination
 
     def get_queryset(self):
         query = self.model.objects.api(self.request.user)
         org = self.request.query_params.get('org', None)
         if org is not None:
             query = query.filter(organization__pk=org)
-        query = query.filter(account__provider__in=[service.adapter.provider_id
-                                                    for service in registry])
+        query = query.filter(
+            account__provider__in=[
+                service.adapter.provider_id for service in registry
+            ])
         return query
-
-    def get_paginate_by(self):
-        return self.request.query_params.get('page_size', 25)

readthedocs/rtd_tests/tests/test_api.py

@@ -265,6 +265,43 @@ class APITests(TestCase):
         self.assertIn('features', resp.data)
         self.assertEqual(resp.data['features'], [feature.feature_id])
 
+    def test_project_pagination(self):
+        for _ in range(100):
+            get(Project)
+
+        resp = self.client.get('/api/v2/project/')
+        self.assertEqual(resp.status_code, 200)
+        self.assertEqual(len(resp.data['results']), 100)  # page_size
+        self.assertIn('?page=2', resp.data['next'])
+
+    def test_remote_repository_pagination(self):
+        account = get(SocialAccount, provider='github')
+        user = get(User, socialaccount_set=[account])
+        for _ in range(20):
+            get(RemoteRepository, users=[user], account=account)
+
+        client = APIClient()
+        client.force_authenticate(user=user)
+
+        resp = client.get('/api/v2/remote/repo/')
+        self.assertEqual(resp.status_code, 200)
+        self.assertEqual(len(resp.data['results']), 15)  # page_size
+        self.assertIn('?page=2', resp.data['next'])
+
+    def test_remote_organization_pagination(self):
+        account = get(SocialAccount, provider='github')
+        user = get(User, socialaccount_set=[account])
+        for _ in range(30):
+            get(RemoteOrganization, users=[user], account=account)
+
+        client = APIClient()
+        client.force_authenticate(user=user)
+
+        resp = client.get('/api/v2/remote/org/')
+        self.assertEqual(resp.status_code, 200)
+        self.assertEqual(len(resp.data['results']), 25)  # page_size
+        self.assertIn('?page=2', resp.data['next'])
+
 
 class APIImportTests(TestCase):