Merge pull request #1701 from coreos-inc/repo-list-query

Fix pagination of public repos, make more efficient and add test

data/model/modelutil.py

@@ -26,11 +26,13 @@ def paginate(query, model, descending=False, page_token=None, limit=50, id_alias
         query = query.where(model.id <= start_id)
       else:
         query = query.where(model.id >= start_id)
+  else:
+    query = query.limit(limit + 1)
 
   results = list(query)
   page_token = None
   if len(results) > limit:
-    start_id = results[limit].id
+    start_id = getattr(results[limit], id_alias or 'id')
     page_token = {
       'start_id': start_id
     }

data/model/repository.py

@@ -256,11 +256,12 @@ def get_visible_repositories(username, namespace=None, include_public=False):
   query = (Repository
            .select(Repository.name, Repository.id.alias('rid'),
                    Repository.description, Namespace.username, Repository.visibility)
-           .distinct()
            .switch(Repository)
-           .join(Namespace, on=(Repository.namespace_user == Namespace.id))
-           .switch(Repository)
-           .join(RepositoryPermission, JOIN_LEFT_OUTER))
+           .join(Namespace, on=(Repository.namespace_user == Namespace.id)))
+
+  if username:
+    # Note: We only need the permissions table if we will filter based on a user's permissions.
+    query = query.switch(Repository).distinct().join(RepositoryPermission, JOIN_LEFT_OUTER)
 
   query = _basequery.filter_to_repos_for_user(query, username, namespace, include_public)
   return query

test/test_api_usage.py

@@ -24,7 +24,7 @@ from app import app, config_provider
 from buildtrigger.basehandler import BuildTriggerHandler
 from initdb import setup_database_for_testing, finished_database_for_testing
 from data import database, model
-from data.database import RepositoryActionCount
+from data.database import RepositoryActionCount, Repository as RepositoryTable
 from test.helpers import assert_action_logged
 
 from endpoints.api.team import TeamMember, TeamMemberList, TeamMemberInvite, OrganizationTeam
@@ -1453,19 +1453,38 @@ class TestListRepos(ApiTestCase):
     self.assertEquals(len(json['repositories']), 1)
 
   def test_listrepos_asguest_withpages(self):
-    # Add public repos until we have enough for over 1 page.
     public_user = model.user.get_user('public')
-    for i in range(0, REPOS_PER_PAGE):
-      model.repository.create_repository('public', 'publicrepo%s' % i, public_user,
-                                         visibility='public')
-    # Request the first page of results.
-    json = self.getJsonResponse(RepositoryList, params=dict(public=True))
-    self.assertEquals(len(json['repositories']), REPOS_PER_PAGE)
 
-    # Request the second page of results.
-    json = self.getJsonResponse(RepositoryList, params=dict(public=True,
-                                                            next_page=json['next_page']))
-    self.assertGreater(len(json['repositories']), 0)
+    # Delete all existing repos under the namespace.
+    for repo in list(RepositoryTable.select().where(RepositoryTable.namespace_user == public_user)):
+      model.repository.purge_repository(public_user.username, repo.name)
+
+    # Add public repos until we have enough for a few pages.
+    required = set()
+    for i in range(0, REPOS_PER_PAGE * 2):
+      name = 'publicrepo%s' % i
+      model.repository.create_repository('public', name, public_user,
+                                         visibility='public')
+      required.add(name)
+
+    # Request results until we no longer have any.
+    next_page = None
+    while True:
+      json = self.getJsonResponse(RepositoryList, params=dict(public=True, next_page=next_page))
+      for repo in json['repositories']:
+        name = repo['name']
+        self.assertTrue(name in required)
+        required.remove(name)
+
+      if 'next_page' in json:
+        self.assertEquals(len(json['repositories']), REPOS_PER_PAGE)
+      else:
+        break
+
+      next_page = json['next_page']
+
+    # Ensure we found all the repositories.
+    self.assertEquals(0, len(required))
 
   def test_listrepos_asorgmember(self):
     self.login(READ_ACCESS_USER)