Merge pull request #1593 from coreos-inc/pagination-error

Fix various SQL errors

data/model/modelutil.py

@@ -1,4 +1,4 @@
-def paginate(query, model, descending=False, page_token=None, limit=50):
+def paginate(query, model, descending=False, page_token=None, limit=50, id_field='id'):
   """ Paginates the given query using an ID range, starting at the optional page_token.
       Returns a *list* of matching results along with an unencrypted page_token for the
       next page, if any. If descending is set to True, orders by the ID descending rather
@@ -7,9 +7,9 @@ def paginate(query, model, descending=False, page_token=None, limit=50):
   query = query.limit(limit + 1)
 
   if descending:
-    query = query.order_by(model.id.desc())
+    query = query.order_by(getattr(model, id_field).desc())
   else:
-    query = query.order_by(model.id)
+    query = query.order_by(getattr(model, id_field))
 
   if page_token is not None:
     start_id = page_token.get('start_id')

data/model/repository.py

@@ -231,10 +231,12 @@ 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:
-    return []
+    # Short circuit by returning a query that will find no repositories. We need to return a query
+    # here, as it will be modified by other queries later on.
+    return Repository.select().where(Repository.id == -1)
 
   query = (Repository
-           .select(Repository.name, Repository.id.alias('id'), Repository.description, Namespace.username,
+           .select(Repository.name, Repository.id.alias('rid'), Repository.description, Namespace.username,
                    Repository.visibility)
            .distinct()
            .switch(Repository)
@@ -394,6 +396,6 @@ def list_popular_public_repos(action_count_threshold, time_span):
           .join(RepositoryActionCount)
           .where(RepositoryActionCount.date >= cutoff,
                  Repository.visibility == get_public_repo_visibility())
-          .group_by(RepositoryActionCount.repository)
+          .group_by(RepositoryActionCount.repository, Repository.name, Namespace.username)
           .having(fn.Sum(RepositoryActionCount.count) >= action_count_threshold)
           .tuples())

endpoints/api/repository.py

@@ -166,7 +166,8 @@ class RepositoryList(ApiResource):
     # result in far smaller queries.
     if not parsed_args['namespace'] and not parsed_args['starred']:
       repos, next_page_token = model.modelutil.paginate(repo_query, repo_query.c,
-                                                        page_token=page_token, limit=REPOS_PER_PAGE)
+                                                        page_token=page_token, limit=REPOS_PER_PAGE,
+                                                        id_field='rid')
     else:
       repos = list(repo_query)
       next_page_token = None

test/test_api_usage.py

@@ -55,7 +55,8 @@ from endpoints.api.organization import (OrganizationList, OrganizationMember,
                                         Organization, ApplicationInformation,
                                         OrganizationApplications, OrganizationApplicationResource,
                                         OrganizationApplicationResetClientSecret, Organization)
-from endpoints.api.repository import RepositoryList, RepositoryVisibility, Repository
+from endpoints.api.repository import (RepositoryList, RepositoryVisibility, Repository,
+                                      REPOS_PER_PAGE)
 from endpoints.api.permission import (RepositoryUserPermission, RepositoryTeamPermission,
                                       RepositoryTeamPermissionList, RepositoryUserPermissionList)
 from endpoints.api.superuser import (SuperUserLogs, SuperUserList, SuperUserManagement,
@@ -1446,13 +1447,24 @@ 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)
+
   def test_listrepos_asorgmember(self):
     self.login(READ_ACCESS_USER)
-
-    # Queries: Base + the list query
-    with assert_query_count(BASE_LOGGEDIN_QUERY_COUNT + 2):
-      json = self.getJsonResponse(RepositoryList, params=dict(public=True))
-
+    json = self.getJsonResponse(RepositoryList, params=dict(public=True))
     self.assertGreater(len(json['repositories']), 0)
 
   def test_listrepos_filter(self):
@@ -1461,7 +1473,7 @@ class TestListRepos(ApiTestCase):
                                 params=dict(namespace=ORGANIZATION,
                                             public=False))
 
-    self.assertTrue(len(json['repositories']) > 0)
+    self.assertGreater(len(json['repositories']), 0)
 
     for repo in json['repositories']:
       self.assertEquals(ORGANIZATION, repo['namespace'])
@@ -1477,7 +1489,7 @@ class TestListRepos(ApiTestCase):
                                               last_modified=True,
                                               popularity=True))
 
-      self.assertTrue(len(json['repositories']) > 0)
+    self.assertGreater(len(json['repositories']), 0)
 
     for repo in json['repositories']:
       self.assertEquals(ORGANIZATION, repo['namespace'])