Merge branch 'avoid_n_plus_1_es_load_blobs' into 'master'

Avoid Project N+1 on Elasticsearch Code results

See merge request gitlab-org/gitlab-ee!10394

app/helpers/search_helper.rb

@@ -33,10 +33,15 @@ module SearchHelper
     "Showing #{from} - #{to} of #{count} #{scope.humanize(capitalize: false)} for \"#{term}\""
   end
 
-  def find_project_for_result_blob(result)
+  def find_project_for_result_blob(projects, result)
     @project
   end
 
+  # Used in EE
+  def blob_projects(results)
+    nil
+  end
+
   def parse_search_result(result)
     result
   end

app/views/search/_results.html.haml

@@ -22,6 +22,8 @@
         - if @scope == 'projects'
           .term
             = render 'shared/projects/list', projects: @search_objects, pipeline_status: false
+        - elsif %w[blobs wiki_blobs].include?(@scope)
+          = render partial: 'search/results/blob', collection: @search_objects, locals: { projects: blob_projects(@search_objects) }
         - else
           = render partial: "search/results/#{@scope.singularize}", collection: @search_objects
 

app/views/search/results/_blob.html.haml

-- project = find_project_for_result_blob(blob)
+- project = find_project_for_result_blob(projects, blob)
 - return unless project
 
 - blob = parse_search_result(blob)

app/views/search/results/_wiki_blob.html.haml

-- project = find_project_for_result_blob(wiki_blob)
+- project = find_project_for_result_blob(projects, wiki_blob)
 - wiki_blob = parse_search_result(wiki_blob)
 - wiki_blob_link = project_wiki_path(project, wiki_blob.basename)
 

ee/app/helpers/ee/search_helper.rb

@@ -11,11 +11,20 @@ module EE
     end
 
     override :find_project_for_result_blob
-    # rubocop: disable CodeReuse/ActiveRecord
-    def find_project_for_result_blob(result)
-      super || ::Project.find_by(id: result.dig('_source', 'join_field', 'parent')&.split('_')&.last)
+    def find_project_for_result_blob(projects, result)
+      return super if result.is_a?(::Gitlab::Search::FoundBlob)
+
+      super || projects&.find { |project| project.id == blob_project_id(result) }
+    end
+
+    override :blob_projects
+    def blob_projects(results)
+      return super if results.first.is_a?(::Gitlab::Search::FoundBlob)
+
+      project_ids = results.map(&method(:blob_project_id))
+
+      ::ProjectsFinder.new(current_user: current_user, project_ids_relation: project_ids).execute
     end
-    # rubocop: enable CodeReuse/ActiveRecord
 
     override :parse_search_result
     def parse_search_result(result)
@@ -41,5 +50,9 @@ module EE
       type == :issues && (context == :dashboard ||
         context.feature_available?(:multiple_issue_assignees))
     end
+
+    def blob_project_id(blob_result)
+      blob_result.dig('_source', 'join_field', 'parent')&.split('_')&.last.to_i
+    end
   end
 end

ee/changelogs/unreleased/avoid_n_plus_1_es_load_blobs.yml

+---
+title: Avoid N+1 when loading Code search results with Elasticsearch enabled
+merge_request: 10394
+author:
+type: performance

ee/spec/helpers/search_helper_spec.rb

@@ -49,17 +49,12 @@ describe SearchHelper do
     end
   end
 
-  describe '#parse_search_result_from_elastic' do
+  describe '#parse_search_result with elastic enabled', :elastic do
     let(:user) { create(:user) }
 
     before do
+      allow(self).to receive(:current_user).and_return(user)
       stub_ee_application_setting(elasticsearch_search: true, elasticsearch_indexing: true)
-      Gitlab::Elastic::Helper.create_empty_index
-    end
-
-    after do
-      Gitlab::Elastic::Helper.delete_index
-      stub_ee_application_setting(elasticsearch_search: false, elasticsearch_indexing: false)
     end
 
     it "returns parsed result" do
@@ -82,28 +77,37 @@ describe SearchHelper do
       expect(parsed_result.startline).to eq(2)
       expect(parsed_result.data).to include("Popen")
     end
+  end
+
+  describe '#blob_projects', :elastic do
+    let(:user) { create(:user) }
+
+    before do
+      allow(self).to receive(:current_user).and_return(user)
+      stub_ee_application_setting(elasticsearch_search: true, elasticsearch_indexing: true)
+    end
 
-    it 'does not return project that does not exist' do
-      Gitlab::Elastic::Helper.create_empty_index
+    def es_blob_search
+      Repository.search(
+        'def popen',
+        type: :blob,
+        options: { highlight: true }
+      )[:blobs][:results]
+    end
 
-      @project_2 = create :project, :repository
+    it 'returns all projects in the result page without causing an N+1' do
+      control_count = ActiveRecord::QueryRecorder.new { blob_projects(es_blob_search) }.count
 
-      @project_2.repository.create_file(
-        user,
-        'thing.txt',
-        ' function application.js ',
-        message: 'Find me',
-        branch_name: 'master')
+      projects = create_list :project, 3, :repository, :public
+      projects.each { |project| project.repository.index_blobs }
 
-      @project_2.repository.index_blobs
       Gitlab::Elastic::Helper.refresh_index
-      @project_2.destroy
-
-      blob = { _source: { join_field: { parent: @project_2.es_id } } }.as_json
 
-      result = find_project_for_result_blob(blob)
+      # So we can access it outside the following block
+      result_projects = nil
 
-      expect(result).to be(nil)
+      expect { result_projects = blob_projects(es_blob_search) }.not_to exceed_query_limit(control_count)
+      expect(result_projects).to match_array(projects)
     end
   end
 end