diff --git a/data/model/repository.py b/data/model/repository.py index f39aacaf9..69274a138 100644 --- a/data/model/repository.py +++ b/data/model/repository.py @@ -184,7 +184,7 @@ def unstar_repository(user, repository): raise DataModelException('Star not found.') -def get_user_starred_repositories(user, limit=None, page=None): +def get_user_starred_repositories(user): """ Retrieves all of the repositories a user has starred. """ query = (Repository .select(Repository, User, Visibility) @@ -195,11 +195,6 @@ def get_user_starred_repositories(user, limit=None, page=None): .join(Visibility) .where(Star.user == user)) - if page and limit: - query = query.paginate(page, limit) - elif limit: - query = query.limit(limit) - return query @@ -252,7 +247,7 @@ def get_action_counts(repository_ids): return action_count_map -def get_visible_repositories(username, namespace=None, page=None, limit=None, include_public=False): +def get_visible_repositories(username, namespace=None, include_public=False): """ Returns the repositories visible to the given user (if any). """ if not include_public and not username: @@ -268,11 +263,6 @@ def get_visible_repositories(username, namespace=None, page=None, limit=None, in .join(RepositoryPermission, JOIN_LEFT_OUTER)) query = _basequery.filter_to_repos_for_user(query, username, namespace, include_public) - if page: - query = query.paginate(page, limit) - elif limit: - query = query.limit(limit) - return query diff --git a/endpoints/api/repository.py b/endpoints/api/repository.py index cecfbfa7c..96ff8b161 100644 --- a/endpoints/api/repository.py +++ b/endpoints/api/repository.py @@ -6,17 +6,15 @@ import features from datetime import timedelta -from flask import request +from flask import request, abort from data import model -from data.database import (Repository as RepositoryTable, Visibility, RepositoryTag, - RepositoryActionCount, Namespace, fn) - +from data.database import Repository as RepositoryTable from endpoints.api import (truthy_bool, format_date, nickname, log_action, validate_json_request, require_repo_read, require_repo_write, require_repo_admin, RepositoryParamResource, resource, query_param, parse_args, ApiResource, request_error, require_scope, Unauthorized, NotFound, InvalidRequest, - path_param, ExceedsLicenseException) + path_param, ExceedsLicenseException, page_support) from endpoints.api.billing import lookup_allowed_private_repos, get_namespace_plan from endpoints.api.subscribe import check_repository_usage @@ -29,6 +27,7 @@ from util.names import REPOSITORY_NAME_REGEX logger = logging.getLogger(__name__) +REPOS_PER_PAGE = 100 def check_allowed_private_repos(namespace): """ Checks to see if the given namespace has reached its private repository limit. If so, @@ -123,53 +122,10 @@ class RepositoryList(ApiResource): raise Unauthorized() - def _load_repositories(self, namespace=None, public=False, starred=False, limit=None, page=None): - """ Loads the filtered list of repositories and returns it and the star lookup set. """ - # Load the starred repositories and username (if applicable) - username = get_authenticated_user().username if get_authenticated_user() else None - - # If starred (and only starred) repositories were requested, then load them directly. - if starred and namespace is None and not public: - if not username: - return [], set() - - repositories = model.repository.get_user_starred_repositories(get_authenticated_user(), - limit=limit, - page=page) - - return repositories, set([repo.id for repo in repositories]) - - # Otherwise, conduct a full filtered lookup and (optionally) filter by the starred repositories. - starred_repos = [] - star_lookup = set() - - if username: - starred_repos = model.repository.get_user_starred_repositories(get_authenticated_user()) - star_lookup = set([repo.id for repo in starred_repos]) - - # If the user asked for only public repositories, limit to only public repos. - if public and (not namespace and not starred): - username = None - - # Find the matching repositories. - repositories = model.repository.get_visible_repositories(username=username, - limit=limit, - page=page, - include_public=public, - namespace=namespace) - - # Filter down to just the starred repositories, if asked. - if starred: - return [repo for repo in repositories if repo.id in star_lookup], star_lookup - - return repositories, star_lookup - @require_scope(scopes.READ_REPO) @nickname('listRepos') @parse_args() - @query_param('page', 'Offset page number. (int)', type=int) - @query_param('limit', 'Limit on the number of results (int)', type=int) @query_param('namespace', 'Filters the repositories returned to this namespace', type=str) @query_param('starred', 'Filters the repositories returned to those starred by the user', type=truthy_bool, default=False) @@ -179,27 +135,58 @@ class RepositoryList(ApiResource): type=truthy_bool, default=False) @query_param('popularity', 'Whether to include the repository\'s popularity metric.', type=truthy_bool, default=False) - def get(self, parsed_args): + @page_support() + def get(self, page_token, parsed_args): """ Fetch the list of repositories visible to the current user under a variety of situations. """ + # Ensure that the user requests either filtered by a namespace, only starred repositories, + # or public repositories. This ensures that the user is not requesting *all* visible repos, + # which can cause a surge in DB CPU usage. if not parsed_args['namespace'] and not parsed_args['starred'] and not parsed_args['public']: raise InvalidRequest('namespace, starred or public are required for this API call') - repositories, star_lookup = self._load_repositories(parsed_args['namespace'], - parsed_args['public'], - parsed_args['starred'], - parsed_args['limit'], - parsed_args['page']) + user = get_authenticated_user() + username = user.username if user else None + repo_query = None + + # Lookup the requested repositories (either starred or non-starred.) + if parsed_args['starred']: + if not username: + # No repositories should be returned, as there is no user. + abort(400) + + repo_query = model.repository.get_user_starred_repositories(user) + else: + repo_query = model.repository.get_visible_repositories(username=username, + include_public=parsed_args['public'], + namespace=parsed_args['namespace']) + + # Note: We only limit repositories when there isn't a namespace or starred filter, as they + # result in far smaller queries. + if not parsed_args['namespace'] and not parsed_args['starred']: + repos, next_page_token = model.modelutil.paginate(repo_query, RepositoryTable, + page_token=page_token, limit=REPOS_PER_PAGE) + else: + repos = list(repo_query) + next_page_token = None # Collect the IDs of the repositories found for subequent lookup of popularity # and/or last modified. - repository_ids = [repo.id for repo in repositories] + if parsed_args['last_modified'] or parsed_args['popularity']: + repository_ids = [repo.id for repo in repos] - if parsed_args['last_modified']: - last_modified_map = model.repository.get_when_last_modified(repository_ids) + if parsed_args['last_modified']: + last_modified_map = model.repository.get_when_last_modified(repository_ids) - if parsed_args['popularity']: - action_count_map = model.repository.get_action_counts(repository_ids) + if parsed_args['popularity']: + action_count_map = model.repository.get_action_counts(repository_ids) + + # Collect the IDs of the repositories that are starred for the user, so we can mark them + # in the returned results. + star_set = set() + if username: + starred_repos = model.repository.get_user_starred_repositories(user) + star_set = {starred.id for starred in starred_repos} def repo_view(repo_obj): repo = { @@ -217,14 +204,14 @@ class RepositoryList(ApiResource): if parsed_args['popularity']: repo['popularity'] = action_count_map.get(repo_id, 0) - if get_authenticated_user(): - repo['is_starred'] = repo_id in star_lookup + if username: + repo['is_starred'] = repo_id in star_set return repo return { - 'repositories': [repo_view(repo) for repo in repositories] - } + 'repositories': [repo_view(repo) for repo in repos] + }, next_page_token @resource('/v1/repository/') diff --git a/endpoints/api/user.py b/endpoints/api/user.py index eb9e0f663..913272b94 100644 --- a/endpoints/api/user.py +++ b/endpoints/api/user.py @@ -11,11 +11,12 @@ from peewee import IntegrityError import features from app import app, billing as stripe, authentication, avatar +from data.database import Repository as RepositoryTable from endpoints.api import (ApiResource, nickname, resource, validate_json_request, request_error, log_action, internal_only, NotFound, require_user_admin, parse_args, query_param, InvalidToken, require_scope, format_date, show_if, license_error, require_fresh_login, path_param, define_json_response, - RepositoryParamResource) + RepositoryParamResource, page_support) from endpoints.api.subscribe import subscribe from endpoints.common import common_login from endpoints.decorators import anon_allowed @@ -30,6 +31,7 @@ from util.useremails import (send_confirmation_email, send_recovery_email, send_ send_password_changed, send_org_recovery_email) from util.names import parse_single_urn +REPOS_PER_PAGE = 100 logger = logging.getLogger(__name__) @@ -833,15 +835,15 @@ class StarredRepositoryList(ApiResource): @nickname('listStarredRepos') @parse_args() - @query_param('page', 'Offset page number. (int)', type=int) - @query_param('limit', 'Limit on the number of results (int)', type=int) @require_user_admin - def get(self, parsed_args): + @page_support() + def get(self, page_token, parsed_args): """ List all starred repositories. """ - page = parsed_args['page'] - limit = parsed_args['limit'] - starred_repos = model.repository.get_user_starred_repositories(get_authenticated_user(), - page=page, limit=limit) + repo_query = model.repository.get_user_starred_repositories(get_authenticated_user()) + + repos, next_page_token = model.modelutil.paginate(repo_query, RepositoryTable, + page_token=page_token, limit=REPOS_PER_PAGE) + def repo_view(repo_obj): return { 'namespace': repo_obj.namespace_user.username, @@ -850,7 +852,7 @@ class StarredRepositoryList(ApiResource): 'is_public': repo_obj.visibility.name == 'public', } - return {'repositories': [repo_view(repo) for repo in starred_repos]} + return {'repositories': [repo_view(repo) for repo in repos]}, next_page_token @require_scope(scopes.READ_REPO) @nickname('createStar') diff --git a/endpoints/v2/catalog.py b/endpoints/v2/catalog.py index c483a8d41..cb59d6cc5 100644 --- a/endpoints/v2/catalog.py +++ b/endpoints/v2/catalog.py @@ -14,9 +14,7 @@ def catalog_search(): url = url_for('v2.catalog_search') username = get_authenticated_user().username if get_authenticated_user() else None - query = model.repository.get_visible_repositories(username, include_public=(username is None), - limit=50) - + query = model.repository.get_visible_repositories(username, include_public=(username is None)) link, query = add_pagination(query, url) response = jsonify({ diff --git a/test/test_api_usage.py b/test/test_api_usage.py index 7fd804dc3..7903fe012 100644 --- a/test/test_api_usage.py +++ b/test/test_api_usage.py @@ -1360,11 +1360,6 @@ class TestListRepos(ApiTestCase): for repo in json['repositories']: self.assertEquals(ORGANIZATION, repo['namespace']) - def test_listrepos_limit(self): - self.login(READ_ACCESS_USER) - json = self.getJsonResponse(RepositoryList, params=dict(limit=1, public=True)) - self.assertEquals(len(json['repositories']), 1) - def test_listrepos_allparams(self): self.login(ADMIN_ACCESS_USER) @@ -1382,12 +1377,11 @@ class TestListRepos(ApiTestCase): self.assertEquals(ORGANIZATION, repo['namespace']) def test_listrepos_starred_nouser(self): - json = self.getJsonResponse(RepositoryList, - params=dict(last_modified=True, - popularity=True, - starred=True)) - - self.assertEquals(len(json['repositories']), 0) + self.getResponse(RepositoryList, + params=dict(last_modified=True, + popularity=True, + starred=True), + expected_code=400) def test_listrepos_starred(self): self.login(ADMIN_ACCESS_USER)