From 1eec6f53b225d0e1dc064a7155945dd9fc23287f Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Thu, 30 Jun 2016 17:31:35 -0400 Subject: [PATCH 1/2] Fix SQL error with pagination around Repositories Fixes #1591 --- data/model/modelutil.py | 6 +++--- data/model/repository.py | 4 ++-- endpoints/api/repository.py | 3 ++- test/test_api_usage.py | 28 ++++++++++++++++++++-------- 4 files changed, 27 insertions(+), 14 deletions(-) diff --git a/data/model/modelutil.py b/data/model/modelutil.py index 197baa83e..bb153e537 100644 --- a/data/model/modelutil.py +++ b/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') diff --git a/data/model/repository.py b/data/model/repository.py index b5b67db16..948d97cac 100644 --- a/data/model/repository.py +++ b/data/model/repository.py @@ -231,10 +231,10 @@ 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 [] + 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) diff --git a/endpoints/api/repository.py b/endpoints/api/repository.py index 5ba6c39db..c81cf33f2 100644 --- a/endpoints/api/repository.py +++ b/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 diff --git a/test/test_api_usage.py b/test/test_api_usage.py index 61686a1b1..a35e7198f 100644 --- a/test/test_api_usage.py +++ b/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']) From 117ccda1cf773124382f4ac2f4dc4d6cded34fee Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Thu, 30 Jun 2016 17:31:46 -0400 Subject: [PATCH 2/2] Fix postgres error in SQL query --- data/model/repository.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/data/model/repository.py b/data/model/repository.py index 948d97cac..13499904f 100644 --- a/data/model/repository.py +++ b/data/model/repository.py @@ -231,6 +231,8 @@ 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: + # 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 @@ -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())