Optimize repository lookup queries to meet the expected maximums (#246)

* Optimize repository lookup queries to meet the expected maximums

We were accidentally looking up more data that strictly allowed

Adds some additional assertions and testing as well

Fixes https://issues.redhat.com/browse/PROJQUAY-439

* Change loading of repositories in the repo view to be paginated

We drop the "card" view and switch to a table-only view, but still
load the full set of repositories

A followup change will begin to change the UI to only load additional
repos when requested

data/model/oci/tag.py

@@ -203,6 +203,7 @@ def get_most_recent_tag_lifetime_start(repository_ids):
         return {}
 
     assert len(repository_ids) > 0 and None not in repository_ids
+    assert len(repository_ids) <= 100
 
     query = (
         Tag.select(Tag.repository, fn.Max(Tag.lifetime_start_ms))

data/model/repository.py

@@ -351,8 +351,10 @@ def get_filtered_matching_repositories(
         if filter_user is None:
             return []
 
+        # NOTE: We add the offset to the limit here to ensure we have enough results
+        # for the take's we conduct below.
         iterator = _filter_repositories_visible_to_user(
-            unfiltered_query, filter_user.id, limit, repo_kind
+            unfiltered_query, filter_user.id, offset + limit, repo_kind
         )
         if offset > 0:
             take(offset, iterator)
@@ -429,7 +431,7 @@ def _get_sorted_matching_repositories(
             Repository.select(*select_fields)
             .join(RepositorySearchScore)
             .where(Repository.state != RepositoryState.MARKED_FOR_DELETION)
-            .order_by(RepositorySearchScore.score.desc())
+            .order_by(RepositorySearchScore.score.desc(), RepositorySearchScore.id)
         )
     else:
         if search_fields is None:
@@ -454,7 +456,7 @@ def _get_sorted_matching_repositories(
             .join(RepositorySearchScore)
             .where(clause)
             .where(Repository.state != RepositoryState.MARKED_FOR_DELETION)
-            .order_by(SQL("score").desc())
+            .order_by(SQL("score").desc(), RepositorySearchScore.id)
         )
 
     if repo_kind is not None:

data/registry_model/interface.py

@@ -198,8 +198,9 @@ class RegistryDataInterface(object):
     @abstractmethod
     def get_most_recent_tag_lifetime_start(self, repository_refs):
         """
-        Returns a map from repository ID to the last modified time ( seconds from epoch, UTC) for
+        Returns a map from repository ID to the last modified time (seconds from epoch, UTC) for
-        each repository in the given repository reference list.
+        each repository in the given repository reference list. There can be a maximum of 100
+        repositories specified, as this is a VERY heavy operation.
         """
 
     @abstractmethod

endpoints/api/repository.py

@@ -56,7 +56,6 @@ from util.parsing import truthy_bool
 
 logger = logging.getLogger(__name__)
 
-REPOS_PER_PAGE = 100
 MAX_DAYS_IN_3_MONTHS = 92
 
 

endpoints/api/repository_models_pre_oci.py

@@ -91,6 +91,7 @@ class PreOCIModel(RepositoryDataInterface):
         popularity,
     ):
         next_page_token = None
+
         # Lookup the requested repositories (either starred or non-starred.)
         if starred:
             # Return the full list of repos starred by the current user that are still visible to them.
@@ -103,17 +104,6 @@ class PreOCIModel(RepositoryDataInterface):
                 user, kind_filter=repo_kind
             )
             repos = [repo for repo in unfiltered_repos if can_view_repo(repo)]
-        elif namespace:
-            # Repositories filtered by namespace do not need pagination (their results are fairly small),
-            # so we just do the lookup directly.
-            repos = list(
-                model.repository.get_visible_repositories(
-                    username=username,
-                    include_public=public,
-                    namespace=namespace,
-                    kind_filter=repo_kind,
-                )
-            )
         else:
             # Determine the starting offset for pagination. Note that we don't use the normal
             # model.modelutil.paginate method here, as that does not operate over UNION queries, which
@@ -128,13 +118,17 @@ class PreOCIModel(RepositoryDataInterface):
                 start_id=start_id,
                 limit=REPOS_PER_PAGE + 1,
                 kind_filter=repo_kind,
+                namespace=namespace,
             )
 
             repos, next_page_token = model.modelutil.paginate_query(
                 repo_query, limit=REPOS_PER_PAGE, sort_field_name="rid"
             )
 
-        # Collect the IDs of the repositories found for subequent lookup of popularity
+        repos = list(repos)
+        assert len(repos) <= REPOS_PER_PAGE
+
+        # Collect the IDs of the repositories found for subsequent lookup of popularity
         # and/or last modified.
         last_modified_map = {}
         action_sum_map = {}

endpoints/api/search.py

@@ -408,6 +408,9 @@ class ConductRepositorySearch(ApiResource):
     @parse_args()
     @query_param("query", "The search query.", type=str, default="")
     @query_param("page", "The page.", type=int, default=1)
+    @query_param(
+        "includeUsage", "Whether to include usage metadata", type=truthy_bool, default=False
+    )
     @nickname("conductRepoSearch")
     def get(self, parsed_args):
         """
@@ -416,7 +419,7 @@ class ConductRepositorySearch(ApiResource):
         query = parsed_args["query"]
         page = min(max(1, parsed_args["page"]), MAX_RESULT_PAGE_COUNT)
         offset = (page - 1) * MAX_PER_PAGE
-        limit = offset + MAX_PER_PAGE + 1
+        limit = MAX_PER_PAGE + 1
 
         username = get_authenticated_user().username if get_authenticated_user() else None
 
@@ -427,27 +430,35 @@ class ConductRepositorySearch(ApiResource):
             )
         )
 
+        assert len(matching_repos) <= limit
+        has_additional = len(matching_repos) > MAX_PER_PAGE
+        matching_repos = matching_repos[0:MAX_PER_PAGE]
+
         # Load secondary information such as last modified time, star count and action count.
-        repository_ids = [repo.id for repo in matching_repos]
+        last_modified_map = None
-        last_modified_map = registry_model.get_most_recent_tag_lifetime_start(matching_repos)
+        star_map = None
-        star_map = model.repository.get_stars(repository_ids)
+        action_sum_map = None
-        action_sum_map = model.log.get_repositories_action_sums(repository_ids)
+        if parsed_args["includeUsage"]:
+            repository_ids = [repo.id for repo in matching_repos]
+            last_modified_map = registry_model.get_most_recent_tag_lifetime_start(matching_repos)
+            star_map = model.repository.get_stars(repository_ids)
+            action_sum_map = model.log.get_repositories_action_sums(repository_ids)
 
         # Build the results list.
         results = [
             repo_result_view(
                 repo,
                 username,
-                last_modified_map.get(repo.id),
+                last_modified_map.get(repo.id) if last_modified_map is not None else None,
-                star_map.get(repo.id, 0),
+                star_map.get(repo.id, 0) if star_map is not None else None,
-                float(action_sum_map.get(repo.id, 0)),
+                float(action_sum_map.get(repo.id, 0)) if action_sum_map is not None else None,
             )
             for repo in matching_repos
         ]
 
         return {
-            "results": results[0:MAX_PER_PAGE],
+            "results": results,
-            "has_additional": len(results) > MAX_PER_PAGE,
+            "has_additional": has_additional,
             "page": page,
             "page_size": MAX_PER_PAGE,
             "start_index": offset,

endpoints/api/test/test_search.py

@@ -3,7 +3,7 @@ import pytest
 from playhouse.test_utils import assert_query_count
 
 from data import model, database
-from endpoints.api.search import ConductRepositorySearch, ConductSearch
+from endpoints.api.search import ConductRepositorySearch, ConductSearch, MAX_PER_PAGE
 from endpoints.api.test.shared import conduct_api_call
 from endpoints.test.shared import client_with_identity
 from test.fixtures import *
@@ -17,7 +17,7 @@ def test_repository_search(query, client):
 
     with client_with_identity("devtable", client) as cl:
         params = {"query": query}
-        with assert_query_count(7):
+        with assert_query_count(4):
             result = conduct_api_call(cl, ConductRepositorySearch, "GET", params, None, 200).json
             assert result["start_index"] == 0
             assert result["page"] == 1
@@ -31,3 +31,37 @@ def test_search_query_count(query, client):
         with assert_query_count(10):
             result = conduct_api_call(cl, ConductSearch, "GET", params, None, 200).json
             assert len(result["results"])
+
+
+@pytest.mark.skipif(
+    os.environ.get("TEST_DATABASE_URI", "").find("mysql") >= 0,
+    reason="MySQL FULLTEXT indexes don't update synchronously",
+)
+@pytest.mark.parametrize("page_count", [1, 2, 4, 6,])
+def test_repository_search_pagination(page_count, client):
+    # Create at least a few pages of results.
+    all_repositories = set()
+    user = model.user.get_user("devtable")
+    for index in range(0, MAX_PER_PAGE * page_count):
+        repo_name = "somerepo%s" % index
+        all_repositories.add(repo_name)
+        model.repository.create_repository("devtable", repo_name, user)
+
+    with client_with_identity("devtable", client) as cl:
+        for page_index in range(0, page_count):
+            params = {"query": "somerepo", "page": page_index + 1}
+
+            repo_results = conduct_api_call(
+                cl, ConductRepositorySearch, "GET", params, None, 200
+            ).json
+            assert len(repo_results["results"]) <= MAX_PER_PAGE
+            for repo in repo_results["results"]:
+                all_repositories.remove(repo["name"])
+
+            if page_index < page_count - 1:
+                assert len(repo_results["results"]) == MAX_PER_PAGE
+                assert repo_results["has_additional"]
+            else:
+                assert not repo_results["has_additional"]
+
+    assert not all_repositories

static/directives/repo-list-view.html

@@ -1,41 +1,8 @@
 <div class="repo-list-view-element">
-  <!-- Toggle -->
-  <div class="repo-list-toggleb btn-group" ng-show="!loading && optionAllowed">
-    <i class="btn btn-default fa fa-th-large" ng-class="!showAsList ? 'active' : ''"
-       ng-click="setShowAsList(false)" title="Grid View" data-container="body" bs-tooltip></i>
-    <i class="btn btn-default fa fa-th-list" ng-class="showAsList ? 'active' : ''"
-       ng-click="setShowAsList(true)" title="List View" data-container="body" bs-tooltip></i>
-  </div>
-
   <div ng-transclude/>
 
-  <!-- Table View -->
+  <div class="repo-list-table" repositories-resources="resources" namespaces="namespaces"
-  <div ng-if="showAsList">
+              star-toggled="starToggled({'repository': repository})"
-    <div class="repo-list-table" repositories-resources="resources" namespaces="namespaces"
+              repo-kind="{{ repoKind }}">
-               star-toggled="starToggled({'repository': repository})"
-               repo-kind="{{ repoKind }}">
-    </div>
-  </div>
-
-  <!-- Grid View -->
-  <div ng-if="!showAsList">
-    <!-- Starred Repository Listing -->
-    <div class="repo-list-grid" repositories-resource="starredRepositories"
-         starred="true"
-         star-toggled="starToggled({'repository': repository})"
-         ng-if="starredRepositories"
-         repo-kind="{{ repoKind }}">
-    </div>
-
-    <!-- User and Org Repository Listings -->
-    <div ng-repeat="namespace in namespaces">
-      <div class="repo-list-grid" repositories-resource="namespace.repositories"
-           starred="false" namespace="namespace"
-           star-toggled="starToggled({'repository': repository})"
-           hide-title="namespaces.length == 1"
-           hide-namespaces="true"
-           repo-kind="{{ repoKind }}">
-      </div>
-    </div>
   </div>
 </div>

static/js/directives/ui/repo-list-table.js

@@ -17,7 +17,7 @@ angular.module('quay').directive('repoListTable', function () {
       $scope.inReadOnlyMode = StateService.inReadOnlyMode();
       $scope.repositories = null;
       $scope.orderedRepositories = [];
-      $scope.reposPerPage = 50;
+      $scope.reposPerPage = 25;
 
       $scope.maxPopularity = 0;
       $scope.options = {

static/js/directives/ui/repo-list-view.js

@@ -18,7 +18,6 @@ angular.module('quay').directive('repoListView', function () {
       $scope.inReadOnlyMode = StateService.inReadOnlyMode();
       $scope.resources = [];
       $scope.loading = true;
-      $scope.showAsList = CookieService.get('quay.repoview') == 'list';
       $scope.optionAllowed = true;
 
       $scope.$watch('namespaces', function(namespaces) {
@@ -34,17 +33,7 @@ angular.module('quay').directive('repoListView', function () {
             }
           }
         });
-
-        $scope.optionAllowed = $scope.resources.length <= 250;
-        if (!$scope.optionAllowed) {
-          $scope.showAsList = true;
-        }
       }, true);
-
-      $scope.setShowAsList = function(value) {
-        $scope.showAsList = value;
-        CookieService.putPermanent('quay.repoview', value ? 'list' : 'grid');
-      };
     }
   };
   return directiveDefinitionObject;

static/js/pages/org-view.js

@@ -44,7 +44,7 @@
         'popularity': true
       };
 
-      $scope.organization.repositories = ApiService.listReposAsResource().withOptions(options).get(function(resp) {
+      $scope.organization.repositories = ApiService.listReposAsResource().withPagination('repositories').withOptions(options).get(function(resp) {
         return resp.repositories;
       });
     };

static/js/pages/repo-list.js

@@ -106,7 +106,7 @@
           'public': namespace.public
         };
 
-        namespace.repositories = ApiService.listReposAsResource().withOptions(options).get(function(resp) {
+        namespace.repositories = ApiService.listReposAsResource().withPagination('repositories').withOptions(options).get(function(resp) {
           return resp.repositories.map(findDuplicateRepo);
         });
 

static/js/pages/search.js

@@ -14,7 +14,8 @@
 
       var params = {
         'query': $routeParams['q'],
-        'page': $scope.currentPage
+        'page': $scope.currentPage,
+        'includeUsage': true
       };
 
       var MAX_PAGE_RESULTS = Config['SEARCH_MAX_RESULT_PAGE_COUNT'];

static/js/pages/user-view.js

@@ -59,7 +59,7 @@
         'popularity': true
       };
 
-      $scope.context.viewuser.repositories = ApiService.listReposAsResource().withOptions(options).get(function(resp) {
+      $scope.context.viewuser.repositories = ApiService.listReposAsResource().withPagination('repositories').withOptions(options).get(function(resp) {
         return resp.repositories;
       });
     };

static/js/services/api-service.js

@@ -10,11 +10,18 @@ angular.module('quay').factory('ApiService', ['Restangular', '$q', 'UtilService'
 
   var getResource = function(getMethod, operation, opt_parameters, opt_background) {
     var resource = {};
+    var paginationKey = null;
+
     resource.withOptions = function(options) {
       this.options = options;
       return this;
     };
 
+    resource.withPagination = function(key) {
+      paginationKey = key;
+      return this;
+    };
+
     resource.get = function(processor, opt_errorHandler) {
       var options = this.options;
       var result = {
@@ -23,17 +30,44 @@ angular.module('quay').factory('ApiService', ['Restangular', '$q', 'UtilService'
         'hasError': false
       };
 
-      getMethod(options, opt_parameters, opt_background, true).then(function(resp) {
+      var paginatedResults = [];
-        result.value = processor(resp);
-        result.loading = false;
-      }, function(resp) {
-        result.hasError = true;
-        result.loading = false;
-        if (opt_errorHandler) {
-          opt_errorHandler(resp);
-        }
-      });
 
+      var performGet = function(opt_nextPageToken) {
+        if (opt_nextPageToken) {
+          opt_parameters = opt_parameters || {};
+          opt_parameters['next_page'] = opt_nextPageToken;
+        }
+
+        getMethod(options, opt_parameters, opt_background, true).then(function(resp) {
+          if (paginationKey) {
+            if (resp && resp[paginationKey]) {
+              Array.prototype.push.apply(paginatedResults, resp[paginationKey]);
+
+              var fullResp = {};
+              fullResp[paginationKey] = paginatedResults;
+              result.value = processor(fullResp);
+              result.loading = resp['next_page'] != null;
+
+              if (result.loading) {
+                performGet(resp['next_page']);
+              }
+
+              return;
+            }
+          }
+
+          result.value = processor(resp);
+          result.loading = false;
+        }, function(resp) {
+          result.hasError = true;
+          result.loading = false;
+          if (opt_errorHandler) {
+            opt_errorHandler(resp);
+          }
+        });
+      };
+
+      performGet();
       return result;
     };
 

test/test_api_usage.py

@@ -117,12 +117,10 @@ from endpoints.api.organization import (
     OrganizationApplicationResetClientSecret,
     Organization,
 )
-from endpoints.api.repository import (
+from endpoints.api.repository import RepositoryList, RepositoryVisibility, Repository
-    RepositoryList,
+
-    RepositoryVisibility,
+from endpoints.api.repository_models_pre_oci import REPOS_PER_PAGE
-    Repository,
+
-    REPOS_PER_PAGE,
-)
 from endpoints.api.permission import (
     RepositoryUserPermission,
     RepositoryTeamPermission,