Merge pull request #1196 from coreos-inc/publicrepoapi

Add pagination to the repository list API to make it better for public

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
 
 

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,
+from data.database import Repository as RepositoryTable
-                           RepositoryActionCount, Namespace, fn)
-
 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'],
+    user = get_authenticated_user()
-                                                        parsed_args['public'],
+    username = user.username if user else None
-                                                        parsed_args['starred'],
+    repo_query = None
-                                                        parsed_args['limit'],
+
-                                                        parsed_args['page'])
+    # 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']:
+      if parsed_args['last_modified']:
-      last_modified_map = model.repository.get_when_last_modified(repository_ids)
+        last_modified_map = model.repository.get_when_last_modified(repository_ids)
 
-    if parsed_args['popularity']:
+      if parsed_args['popularity']:
-      action_count_map = model.repository.get_action_counts(repository_ids)
+        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():
+      if username:
-        repo['is_starred'] = repo_id in star_lookup
+        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/<apirepopath:repository>')

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']
+    repo_query = model.repository.get_user_starred_repositories(get_authenticated_user())
-    limit = parsed_args['limit']
+
-    starred_repos = model.repository.get_user_starred_repositories(get_authenticated_user(),
+    repos, next_page_token = model.modelutil.paginate(repo_query, RepositoryTable,
-                                                                   page=page, limit=limit)
+                                                      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')

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),
+  query = model.repository.get_visible_repositories(username, include_public=(username is None))
-                                                    limit=50)
-
   link, query = add_pagination(query, url)
 
   response = jsonify({

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,
+    self.getResponse(RepositoryList,
-                                params=dict(last_modified=True,
+                     params=dict(last_modified=True,
-                                            popularity=True,
+                                 popularity=True,
-                                            starred=True))
+                                 starred=True),
-
+                     expected_code=400)
-    self.assertEquals(len(json['repositories']), 0)
 
   def test_listrepos_starred(self):
     self.login(ADMIN_ACCESS_USER)