From 9d97e053b3384adc12f3435c2f4fec611c5ca41c Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Wed, 3 May 2017 18:38:46 -0400 Subject: [PATCH 1/2] Make sure to re-sort the filtered repositories in search The filtering breaks the ordered we expected, so we need to re-sort --- data/model/repository.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/data/model/repository.py b/data/model/repository.py index 1b113fe42..e94b8b4fa 100644 --- a/data/model/repository.py +++ b/data/model/repository.py @@ -464,7 +464,13 @@ def _filter_repositories_visible_to_username(unfiltered_query, filter_username, .where(Repository.id << list(new_unfiltered_ids))) filtered = _basequery.filter_to_repos_for_user(query, filter_username, repo_kind=repo_kind) - for filtered_repo in filtered: + + # Sort the filtered repositories by their initial order. + all_filtered_repos = list(filtered) + all_filtered_repos.sort(key=lambda repo: found_ids.index(repo.id)) + + # Yield the repositories in sorted order. + for filtered_repo in all_filtered_repos: yield filtered_repo # If the number of found IDs is less than the chunk count, then we're done. From 227aa8ab8c8130a584a4a8da2e22444e203136f5 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Thu, 4 May 2017 11:25:57 -0400 Subject: [PATCH 2/2] Ensure that search doesn't make extra SQL lookups Before this change, we were accessing the `.kind` on the repository object, which caused peewee to make an extra lookup for each result --- endpoints/api/search.py | 3 ++- endpoints/api/test/test_search.py | 39 ++++++++++++++++++++++++------- 2 files changed, 33 insertions(+), 9 deletions(-) diff --git a/endpoints/api/search.py b/endpoints/api/search.py index 68d927d85..8fe3de3ba 100644 --- a/endpoints/api/search.py +++ b/endpoints/api/search.py @@ -3,6 +3,7 @@ from endpoints.api import (ApiResource, parse_args, query_param, truthy_bool, nickname, resource, require_scope, path_param, internal_only, Unauthorized, InvalidRequest, show_if) +from data.database import Repository from data import model from auth.permissions import (OrganizationMemberPermission, ReadRepositoryPermission, UserAdminPermission, AdministerOrganizationPermission, @@ -266,7 +267,7 @@ def conduct_robot_search(username, query, results): def repo_result_view(repo, username, last_modified=None, stars=None, popularity=None): - kind = 'application' if repo.kind.name == 'application' else 'repository' + kind = 'application' if Repository.kind.get_name(repo.kind_id) == 'application' else 'repository' view = { 'kind': kind, 'title': 'app' if kind == 'application' else 'repo', diff --git a/endpoints/api/test/test_search.py b/endpoints/api/test/test_search.py index 212377352..4efba0841 100644 --- a/endpoints/api/test/test_search.py +++ b/endpoints/api/test/test_search.py @@ -1,12 +1,35 @@ -from endpoints.api.search import ConductRepositorySearch +import pytest + +from playhouse.test_utils import assert_query_count + +from data.model import _basequery +from endpoints.api.search import ConductRepositorySearch, ConductSearch from endpoints.api.test.shared import client_with_identity, conduct_api_call from test.fixtures import * -def test_repository_search(client): +@pytest.mark.parametrize('query, expected_query_count', [ + ('simple', 7), + ('public', 6), + ('repository', 6), +]) +def test_repository_search(query, expected_query_count, client): with client_with_identity('devtable', client) as cl: - params = {'query': 'simple'} - result = conduct_api_call(cl, ConductRepositorySearch, 'GET', params, None, 200).json - assert not result['has_additional'] - assert result['start_index'] == 0 - assert result['page'] == 1 - assert result['results'][0]['name'] == 'simple' + params = {'query': query} + with assert_query_count(expected_query_count): + result = conduct_api_call(cl, ConductRepositorySearch, 'GET', params, None, 200).json + assert result['start_index'] == 0 + assert result['page'] == 1 + assert len(result['results']) + + +@pytest.mark.parametrize('query, expected_query_count', [ + ('simple', 8), + ('public', 8), + ('repository', 8), +]) +def test_search_query_count(query, expected_query_count, client): + with client_with_identity('devtable', client) as cl: + params = {'query': query} + with assert_query_count(expected_query_count): + result = conduct_api_call(cl, ConductSearch, 'GET', params, None, 200).json + assert len(result['results'])