From e252ee07cb75385b6853ade8d74535718faffaf9 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Wed, 6 Jul 2016 16:15:54 -0400 Subject: [PATCH 1/2] Fix popularity metrics on list repos API --- data/model/modelutil.py | 6 +++--- data/model/repository.py | 8 ++++---- endpoints/api/repository.py | 4 ++-- test/test_api_usage.py | 11 +++++++++-- 4 files changed, 18 insertions(+), 11 deletions(-) diff --git a/data/model/modelutil.py b/data/model/modelutil.py index bb153e537..af1e6d123 100644 --- a/data/model/modelutil.py +++ b/data/model/modelutil.py @@ -15,15 +15,15 @@ def paginate(query, model, descending=False, page_token=None, limit=50, id_field start_id = page_token.get('start_id') if start_id is not None: if descending: - query = query.where(model.id <= start_id) + query = query.where(getattr(model, id_field) <= start_id) else: - query = query.where(model.id >= start_id) + query = query.where(getattr(model, id_field) >= start_id) results = list(query) page_token = None if len(results) > limit: page_token = { - 'start_id': results[limit].id + 'start_id': getattr(results[limit], id_field) } return results[0:limit], page_token diff --git a/data/model/repository.py b/data/model/repository.py index 13499904f..9ddf4e6d8 100644 --- a/data/model/repository.py +++ b/data/model/repository.py @@ -187,7 +187,7 @@ def unstar_repository(user, repository): def get_user_starred_repositories(user): """ Retrieves all of the repositories a user has starred. """ query = (Repository - .select(Repository, User, Visibility) + .select(Repository, User, Visibility, Repository.id.alias('rid')) .join(Star) .switch(Repository) .join(User) @@ -233,11 +233,11 @@ def get_visible_repositories(username, namespace=None, include_public=False): 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) + return Repository.select(Repository.id.alias('rid')).where(Repository.id == -1) query = (Repository - .select(Repository.name, Repository.id.alias('rid'), Repository.description, Namespace.username, - Repository.visibility) + .select(Repository.name, Repository.id.alias('rid'), + Repository.description, Namespace.username, Repository.visibility) .distinct() .switch(Repository) .join(Namespace, on=(Repository.namespace_user == Namespace.id)) diff --git a/endpoints/api/repository.py b/endpoints/api/repository.py index c81cf33f2..aab57ea66 100644 --- a/endpoints/api/repository.py +++ b/endpoints/api/repository.py @@ -175,7 +175,7 @@ class RepositoryList(ApiResource): # Collect the IDs of the repositories found for subequent lookup of popularity # and/or last modified. if parsed_args['last_modified'] or parsed_args['popularity']: - repository_ids = [repo.id for repo in repos] + repository_ids = [repo.rid for repo in repos] if parsed_args['last_modified']: last_modified_map = model.repository.get_when_last_modified(repository_ids) @@ -198,7 +198,7 @@ class RepositoryList(ApiResource): 'is_public': repo_obj.visibility_id == model.repository.get_public_repo_visibility().id, } - repo_id = repo_obj.id + repo_id = repo_obj.rid if parsed_args['last_modified']: repo['last_modified'] = last_modified_map.get(repo_id) diff --git a/test/test_api_usage.py b/test/test_api_usage.py index a35e7198f..a037cc3fd 100644 --- a/test/test_api_usage.py +++ b/test/test_api_usage.py @@ -1479,6 +1479,10 @@ class TestListRepos(ApiTestCase): self.assertEquals(ORGANIZATION, repo['namespace']) def test_listrepos_allparams(self): + # Add a repository action count entry for one of the org repos. + repo = model.repository.get_repository(ORGANIZATION, ORG_REPO) + RepositoryActionCount.create(repository=repo, count=10, date=datetime.datetime.utcnow()) + self.login(ADMIN_ACCESS_USER) # Queries: Base + the list query + the popularity and last modified queries + full perms load @@ -1491,8 +1495,11 @@ class TestListRepos(ApiTestCase): self.assertGreater(len(json['repositories']), 0) - for repo in json['repositories']: - self.assertEquals(ORGANIZATION, repo['namespace']) + for repository in json['repositories']: + self.assertEquals(ORGANIZATION, repository['namespace']) + if repository['name'] == ORG_REPO: + self.assertGreater(repository['popularity'], 0) + def test_listrepos_starred_nouser(self): self.getResponse(RepositoryList, From d1699e75b72f296c8aae522c8ee055da43d8daf5 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Wed, 6 Jul 2016 16:17:02 -0400 Subject: [PATCH 2/2] Add missing constructor argument --- health/healthcheck.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/health/healthcheck.py b/health/healthcheck.py index 87a5845ae..50040d97f 100644 --- a/health/healthcheck.py +++ b/health/healthcheck.py @@ -82,9 +82,9 @@ class LocalHealthCheck(HealthCheck): class RDSAwareHealthCheck(HealthCheck): - def __init__(self, app, config_provider, access_key, secret_key, db_instance='quay', - region='us-east-1'): - super(RDSAwareHealthCheck, self).__init__(app, config_provider, ['redis']) + def __init__(self, app, config_provider, instance_keys, access_key, secret_key, + db_instance='quay', region='us-east-1'): + super(RDSAwareHealthCheck, self).__init__(app, config_provider, instance_keys, ['redis']) self.access_key = access_key self.secret_key = secret_key self.db_instance = db_instance