Merge branch '215708-fix-missing-projects-scope' into 'master'

Resolve "Improve performance of Search API (Advanced): commits scope"

See merge request gitlab-org/gitlab!35449

app/models/project.rb

@@ -525,6 +525,10 @@ class Project < ApplicationRecord
             group: :ip_restrictions, namespace: [:route, :owner])
   }
 
+  scope :with_api_commit_entity_associations, -> {
+    preload(:project_feature, :route, namespace: [:route, :owner])
+  }
+
   enum auto_cancel_pending_pipelines: { disabled: 0, enabled: 1 }
 
   chronic_duration_attr :build_timeout_human_readable, :build_timeout,

ee/changelogs/unreleased/215708-fix-missing-projects-scope.yml

+---
+title: Fix N+1 queries for Elastic Search commits scope.
+merge_request: 35449
+author:
+type: performance

ee/lib/elastic/latest/git_class_proxy.rb

@@ -181,7 +181,7 @@ module Elastic
       # Wrap returned results into GitLab model objects and paginate
       #
       # @return [Kaminari::PaginatableArray]
-      def elastic_search_and_wrap(query, type:, page: 1, per: 20, options: {}, &blk)
+      def elastic_search_and_wrap(query, type:, page: 1, per: 20, options: {}, preload_method: nil, &blk)
         response = elastic_search(
           query,
           type: type,
@@ -190,17 +190,19 @@ module Elastic
           options: options
         )[type.pluralize.to_sym][:results]
 
-        items, total_count = yield_each_search_result(response, type, &blk)
+        items, total_count = yield_each_search_result(response, type, preload_method, &blk)
 
         # Before "map" we had a paginated array so we need to recover it
         offset = per * ((page || 1) - 1)
         Kaminari.paginate_array(items, total_count: total_count, limit: per, offset: offset)
       end
 
-      def yield_each_search_result(response, type)
+      def yield_each_search_result(response, type, preload_method)
         # Avoid one SELECT per result by loading all projects into a hash
         project_ids = response.map { |result| project_id_for_commit_or_blob(result, type) }.uniq
-        projects = Project.with_route.id_in(project_ids).index_by(&:id)
+        projects = Project.with_route.id_in(project_ids)
+        projects = projects.public_send(preload_method) if preload_method # rubocop:disable GitlabSecurity/PublicSend
+        projects = projects.index_by(&:id)
         total_count = response.total_count
 
         items = response.map do |result|

ee/lib/elastic/latest/repository_class_proxy.rb

@@ -10,8 +10,8 @@ module Elastic
       end
 
       # @return [Kaminari::PaginatableArray]
-      def find_commits_by_message_with_elastic(query, page: 1, per_page: 20, options: {})
-        elastic_search_and_wrap(query, type: 'commit', page: page, per: per_page, options: options) do |result, project|
+      def find_commits_by_message_with_elastic(query, page: 1, per_page: 20, options: {}, preload_method: nil)
+        elastic_search_and_wrap(query, type: 'commit', page: page, per: per_page, options: options, preload_method: preload_method) do |result, project|
           raw_commit = Gitlab::Git::Commit.new(
             project.repository.raw,
             prepare_commit(result['_source']['commit']),

ee/lib/elastic/latest/repository_instance_proxy.rb

@@ -8,7 +8,7 @@ module Elastic
       delegate :project, to: :target
       delegate :id, to: :project, prefix: true
 
-      def find_commits_by_message_with_elastic(query, page: 1, per_page: 20)
+      def find_commits_by_message_with_elastic(query, page: 1, per_page: 20, preload_method: nil)
         response = elastic_search(query, type: 'commit', page: page, per: per_page)[:commits][:results]
 
         commits = response.map do |result|

ee/lib/gitlab/elastic/project_search_results.rb

@@ -61,7 +61,7 @@ module Gitlab
         Note.elastic_search(query, options: opt)
       end
 
-      def commits(page: 1, per_page: DEFAULT_PER_PAGE)
+      def commits(page: 1, per_page: DEFAULT_PER_PAGE, preload_method: nil)
         return Kaminari.paginate_array([]) unless Ability.allowed?(@current_user, :download_code, project)
 
         if project.empty_repo? || query.blank?
@@ -72,7 +72,8 @@ module Gitlab
             project.repository.find_commits_by_message_with_elastic(
               query,
               page: (page || 1).to_i,
-              per_page: per_page
+              per_page: per_page,
+              preload_method: preload_method
             )
           else
             offset = per_page * ((page || 1) - 1)

ee/lib/gitlab/elastic/search_results.rb

@@ -43,7 +43,7 @@ module Gitlab
         when 'wiki_blobs'
           wiki_blobs(page: page, per_page: per_page)
         when 'commits'
-          commits(page: page, per_page: per_page)
+          commits(page: page, per_page: per_page, preload_method: preload_method)
         when 'users'
           users.page(page).per(per_page)
         else
@@ -270,7 +270,7 @@ module Gitlab
       # hitting ES twice for any page that's not page 1, and that's something we want to avoid.
       #
       # It is safe to memoize the page we get here because this method is _always_ called before `#commits_count`
-      def commits(page: 1, per_page: DEFAULT_PER_PAGE)
+      def commits(page: 1, per_page: DEFAULT_PER_PAGE, preload_method: nil)
         return Kaminari.paginate_array([]) if query.blank?
 
         strong_memoize(:commits) do
@@ -282,7 +282,8 @@ module Gitlab
             query,
             page: (page || 1).to_i,
             per_page: per_page,
-            options: options
+            options: options,
+            preload_method: preload_method
           )
         end
       end

ee/spec/requests/api/search_spec.rb

@@ -102,7 +102,7 @@ RSpec.describe API::Search do
       it_behaves_like 'pagination', scope: 'wiki_blobs'
     end
 
-    context 'for commits scope', :sidekiq_might_not_need_inline do
+    context 'for commits scope', :sidekiq_inline do
       before do
         project.repository.index_commits_and_blobs
         ensure_elasticsearch_index!
@@ -113,6 +113,20 @@ RSpec.describe API::Search do
       it_behaves_like 'response is correct', schema: 'public_api/v4/commits_details', size: 2
 
       it_behaves_like 'pagination', scope: 'commits'
+
+      it 'avoids N+1 queries' do
+        control = ActiveRecord::QueryRecorder.new { get api(endpoint, user), params: { scope: 'commits', search: 'folder' } }
+
+        project_2 = create(:project, :public, :repository, :wiki_repo, name: 'awesome project 2')
+        project_3 = create(:project, :public, :repository, :wiki_repo, name: 'awesome project 3')
+        project_2.repository.index_commits_and_blobs
+        project_3.repository.index_commits_and_blobs
+
+        ensure_elasticsearch_index!
+
+        # Some N+1 queries still exist
+        expect { get api(endpoint, user), params: { scope: 'commits', search: 'folder' } }.not_to exceed_query_limit(control.count + 9)
+      end
     end
 
     context 'for blobs scope', :sidekiq_might_not_need_inline do

lib/api/search.rb

@@ -24,7 +24,8 @@ module API
         merge_requests: :with_api_entity_associations,
         projects: :with_api_entity_associations,
         issues: :with_api_entity_associations,
-        milestones: :with_api_entity_associations
+        milestones: :with_api_entity_associations,
+        commits: :with_api_commit_entity_associations
       }.freeze
 
       def search(additional_params = {})