From 37d97c0e3d37e29d531c97d957c3c6f3e80a1662 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Sat, 27 Jan 2018 14:30:56 -0500 Subject: [PATCH 1/4] Use DRF 3.1 `pagination_class` --- readthedocs/restapi/utils.py | 15 +++++++++++++++ readthedocs/restapi/views/model_views.py | 11 ++++------- 2 files changed, 19 insertions(+), 7 deletions(-) diff --git a/readthedocs/restapi/utils.py b/readthedocs/restapi/utils.py index b80190bc427..28f738c5a52 100644 --- a/readthedocs/restapi/utils.py +++ b/readthedocs/restapi/utils.py @@ -4,6 +4,8 @@ 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 @@ -167,3 +169,16 @@ def index_search_request(version, page_list, commit, project_scale, page_scale, } } 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 diff --git a/readthedocs/restapi/views/model_views.py b/readthedocs/restapi/views/model_views.py index 24d99980fb2..589c1747fe7 100644 --- a/readthedocs/restapi/views/model_views.py +++ b/readthedocs/restapi/views/model_views.py @@ -64,9 +64,7 @@ class ProjectViewSet(UserSelectViewSet): 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): @@ -241,7 +239,8 @@ 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) @@ -254,6 +253,7 @@ 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) @@ -263,6 +263,3 @@ def get_queryset(self): 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) From 8ba9e1ab15f55eb5e2305bc9abd8fe9b09641cb1 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Sat, 27 Jan 2018 14:56:57 -0500 Subject: [PATCH 2/4] Update pre-commit and styles This style corresponds to PR #3559 --- .pre-commit-config.yaml | 7 +- readthedocs/restapi/utils.py | 112 ++++++++++++++--------- readthedocs/restapi/views/model_views.py | 101 +++++++++++--------- 3 files changed, 132 insertions(+), 88 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 732826af272..93decbcd727 100644 --- a/.pre-commit-config.yaml +++ b/.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'] diff --git a/readthedocs/restapi/utils.py b/readthedocs/restapi/utils.py index 28f738c5a52..43197d98d2f 100644 --- a/readthedocs/restapi/utils.py +++ b/readthedocs/restapi/utils.py @@ -1,6 +1,9 @@ +# -*- 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 @@ -33,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( @@ -51,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 @@ -72,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. @@ -91,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() @@ -114,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({ @@ -131,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, @@ -144,29 +159,40 @@ 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) diff --git a/readthedocs/restapi/views/model_views.py b/readthedocs/restapi/views/model_views.py index 589c1747fe7..348645dec17 100644 --- a/readthedocs/restapi/views/model_views.py +++ b/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,7 +59,7 @@ def get_queryset(self): class ProjectViewSet(UserSelectViewSet): - """List, filter, etc. Projects.""" + """List, filter, etc, Projects.""" permission_classes = [APIPermission] renderer_classes = (JSONRenderer,) @@ -71,15 +73,20 @@ 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, }) @@ -88,7 +95,7 @@ def valid_versions(self, request, **kwargs): 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() @@ -98,7 +105,7 @@ def subprojects(self, request, **kwargs): 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() @@ -107,7 +114,7 @@ def active_versions(self, request, **kwargs): 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]) @@ -116,7 +123,7 @@ def token(self, request, **kwargs): 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() @@ -124,16 +131,18 @@ def canonical_url(self, request, **kwargs): 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']) @@ -160,22 +169,27 @@ def sync_versions(self, request, **kwargs): # noqa: D205 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,11 +255,12 @@ class RemoteOrganizationViewSet(viewsets.ReadOnlyModelViewSet): model = RemoteOrganization 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): @@ -260,6 +275,8 @@ def get_queryset(self): 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 From 87f046ae3f405ac21c1e34e1f71d99cc92d2e105 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Sat, 27 Jan 2018 14:58:37 -0500 Subject: [PATCH 3/4] Lint --- readthedocs/restapi/views/model_views.py | 1 - 1 file changed, 1 deletion(-) diff --git a/readthedocs/restapi/views/model_views.py b/readthedocs/restapi/views/model_views.py index 589c1747fe7..d817897b348 100644 --- a/readthedocs/restapi/views/model_views.py +++ b/readthedocs/restapi/views/model_views.py @@ -241,7 +241,6 @@ class RemoteOrganizationViewSet(viewsets.ReadOnlyModelViewSet): model = RemoteOrganization 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 From 323d650c293a57dc0d45d6262bd95196973a8393 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 30 Jan 2018 10:55:07 -0500 Subject: [PATCH 4/4] Pagination tests --- readthedocs/rtd_tests/tests/test_api.py | 37 +++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/readthedocs/rtd_tests/tests/test_api.py b/readthedocs/rtd_tests/tests/test_api.py index b4285500729..d2f9c6e4b53 100644 --- a/readthedocs/rtd_tests/tests/test_api.py +++ b/readthedocs/rtd_tests/tests/test_api.py @@ -265,6 +265,43 @@ def test_project_features_multiple_projects(self): 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):