Merge branch '227383-fix-commits-search-n-plus-1' into 'master'

Fix N+1 for commits with pipelines

See merge request gitlab-org/gitlab!59234

app/controllers/concerns/renders_commits.rb

@@ -23,6 +23,7 @@ module RendersCommits
 
   def prepare_commits_for_rendering(commits)
     commits.each(&:lazy_author) # preload commits' authors
+    commits.each(&:lazy_latest_pipeline)
 
     Banzai::CommitRenderer.render(commits, @project, current_user) # rubocop:disable Gitlab/ModuleWithInstanceVariables
 

app/models/ci/commit_with_pipeline.rb

@@ -18,9 +18,25 @@ class Ci::CommitWithPipeline < SimpleDelegator
     end
   end
 
+  def lazy_latest_pipeline
+    BatchLoader.for(sha).batch do |shas, loader|
+      preload_pipelines = project.ci_pipelines.latest_pipeline_per_commit(shas.compact)
+
+      shas.each do |sha|
+        pipeline = preload_pipelines[sha]
+
+        loader.call(sha, pipeline)
+      end
+    end
+  end
+
   def latest_pipeline(ref = nil)
     @latest_pipelines.fetch(ref) do |ref|
-      @latest_pipelines[ref] = latest_pipeline_for_project(ref, project)
+      @latest_pipelines[ref] = if ref
+                                 latest_pipeline_for_project(ref, project)
+                               else
+                                 lazy_latest_pipeline&.itself
+                               end
     end
   end
 

app/models/commit.rb

@@ -142,6 +142,7 @@ class Commit
   delegate \
     :pipelines,
     :last_pipeline,
+    :lazy_latest_pipeline,
     :latest_pipeline,
     :latest_pipeline_for_project,
     :set_latest_pipeline_for_ref,

changelogs/unreleased/227383-fix-commits-search-n-plus-1.yml

+---
+title: Fix N+1 for commits with pipelines
+merge_request: 59234
+author:
+type: performance

ee/spec/features/search/elastic/global_search_spec.rb

@@ -102,6 +102,23 @@ RSpec.describe 'Global elastic search', :elastic, :sidekiq_inline do
         expect(page).to have_css('.search-results') # Confirm there are search results to prevent false positives
       end
     end
+
+    context 'searching commits' do
+      let(:path_for_one) { search_path(search: '*', scope: 'commits', per_page: 1) }
+      let(:path_for_multiple) { search_path(search: '*', scope: 'commits', per_page: 5) }
+
+      it 'avoids N+1 database queries' do
+        project.repository.index_commits_and_blobs
+
+        ensure_elasticsearch_index!
+
+        control = ActiveRecord::QueryRecorder.new { visit path_for_one }
+        expect(page).to have_css('.results') # Confirm there are search results to prevent false positives
+
+        expect { visit path_for_multiple }.not_to exceed_query_limit(control.count).with_threshold(2) # We still have users N+1 here
+        expect(page).to have_css('.results') # Confirm there are search results to prevent false positives
+      end
+    end
   end
 
   describe 'I search through the issues and I see pagination' do

spec/controllers/concerns/renders_commits_spec.rb

@@ -66,6 +66,7 @@ RSpec.describe RendersCommits do
 
       expect do
         subject.prepare_commits_for_rendering(merge_request.commits)
+        merge_request.commits.each(&:latest_pipeline)
       end.not_to exceed_all_query_limit(control.count)
     end
   end

spec/models/ci/commit_with_pipeline_spec.rb

@@ -26,15 +26,47 @@ RSpec.describe Ci::CommitWithPipeline do
     end
   end
 
+  describe '#lazy_latest_pipeline' do
+    let(:commit_1) do
+      described_class.new(Commit.new(RepoHelpers.sample_commit, project))
+    end
+
+    let(:commit_2) do
+      described_class.new(Commit.new(RepoHelpers.another_sample_commit, project))
+    end
+
+    let!(:commits) { [commit_1, commit_2] }
+
+    it 'executes only 1 SQL query' do
+      recorder = ActiveRecord::QueryRecorder.new do
+        # Running this first ensures we don't run one query for every
+        # commit.
+        commits.each(&:lazy_latest_pipeline)
+
+        # This forces the execution of the SQL queries necessary to load the
+        # data.
+        commits.each { |c| c.latest_pipeline.try(:id) }
+      end
+
+      expect(recorder.count).to eq(1)
+    end
+  end
+
   describe '#latest_pipeline' do
     let(:pipeline) { double }
 
     shared_examples_for 'fetching latest pipeline' do |ref|
       it 'returns the latest pipeline for the project' do
-        expect(commit)
-          .to receive(:latest_pipeline_for_project)
-          .with(ref, project)
-          .and_return(pipeline)
+        if ref
+          expect(commit)
+            .to receive(:latest_pipeline_for_project)
+            .with(ref, project)
+            .and_return(pipeline)
+        else
+          expect(commit)
+            .to receive(:lazy_latest_pipeline)
+            .and_return(pipeline)
+        end
 
         expect(result).to eq(pipeline)
       end

spec/services/ci/destroy_pipeline_service_spec.rb

@@ -24,6 +24,9 @@ RSpec.describe ::Ci::DestroyPipelineService do
 
       subject
 
+      # We need to reset lazy_latest_pipeline cache to simulate a new request
+      BatchLoader::Executor.clear_current
+
       # Need to use find to avoid memoization
       expect(Project.find(project.id).pipeline_status.has_status?).to be_falsey
     end

spec/services/ci/expire_pipeline_cache_service_spec.rb

@@ -60,6 +60,9 @@ RSpec.describe Ci::ExpirePipelineCacheService do
 
         pipeline_with_commit.destroy!
 
+        # We need to reset lazy_latest_pipeline cache to simulate a new request
+        BatchLoader::Executor.clear_current
+
         # Need to use find to avoid memoization
         expect(Project.find(project_with_repo.id).pipeline_status.has_status?).to be_falsey
       end