Merge branch '330707-pipeline-show-jobs-nplusone' into 'master'

Eliminate N+1 queries for pipeline show jobs

See merge request gitlab-org/gitlab!68743

app/presenters/ci/build_presenter.rb

@@ -12,11 +12,11 @@ module Ci
       erased_by.name if erased_by_user?
     end
 
-    def status_title
+    def status_title(status = detailed_status)
       if auto_canceled?
         "Job is redundant and is auto-canceled by Pipeline ##{auto_canceled_by_id}"
       else
-        tooltip_for_badge
+        tooltip_for_badge(status)
       end
     end
 
@@ -41,8 +41,8 @@ module Ci
 
     private
 
-    def tooltip_for_badge
-      detailed_status.badge_tooltip.capitalize
+    def tooltip_for_badge(status)
+      status.badge_tooltip.capitalize
     end
 
     def detailed_status

app/presenters/ci/stage_presenter.rb

@@ -15,18 +15,23 @@ module Ci
     private
 
     def preload_statuses(statuses)
-      loaded_statuses = statuses.load
-      statuses.tap do |statuses|
-        # rubocop: disable CodeReuse/ActiveRecord
-        ActiveRecord::Associations::Preloader.new.preload(preloadable_statuses(loaded_statuses), %w[pipeline tags job_artifacts_archive metadata])
-        # rubocop: enable CodeReuse/ActiveRecord
-      end
-    end
+      common_relations = [:pipeline]
 
-    def preloadable_statuses(statuses)
-      statuses.reject do |status|
-        status.instance_of?(::GenericCommitStatus) || status.instance_of?(::Ci::Bridge)
+      preloaders = {
+        ::Ci::Build => [:metadata, :tags, :job_artifacts_archive],
+        ::Ci::Bridge => [:metadata, :downstream_pipeline],
+        ::GenericCommitStatus => []
+      }
+
+      # rubocop: disable CodeReuse/ActiveRecord
+      preloaders.each do |klass, relations|
+        ActiveRecord::Associations::Preloader
+          .new
+          .preload(statuses.select { |job| job.is_a?(klass) }, relations + common_relations)
       end
+      # rubocop: enable CodeReuse/ActiveRecord
+
+      statuses
     end
   end
 end

app/views/projects/ci/builds/_build.html.haml

@@ -7,10 +7,14 @@
 - pipeline_link = local_assigns.fetch(:pipeline_link, false)
 - stage = local_assigns.fetch(:stage, false)
 - allow_retry = local_assigns.fetch(:allow_retry, false)
+-# This prevents initializing another Ci::Status object where 'status' is used
+- status = job.detailed_status(current_user)
 
 %tr.build.commit{ class: ('retried' if retried) }
   %td.status
-    = render "ci/status/badge", status: job.detailed_status(current_user), title: job.status_title
+    -# Sending 'status' prevents calling the user relation inside the presenter, generating N+1,
+    -# see https://gitlab.com/gitlab-org/gitlab/-/merge_requests/68743
+    = render "ci/status/badge", status: status, title: job.status_title(status)
 
   %td
     - if can?(current_user, :read_build, job)

spec/controllers/projects/pipelines_controller_spec.rb

@@ -311,23 +311,42 @@ RSpec.describe Projects::PipelinesController do
 
       let_it_be(:pipeline) { create(:ci_pipeline, project: project) }
 
-      def create_build_with_artifacts(stage, stage_idx, name)
-        create(:ci_build, :artifacts, :tags, pipeline: pipeline, stage: stage, stage_idx: stage_idx, name: name)
+      def create_build_with_artifacts(stage, stage_idx, name, status)
+        create(:ci_build, :artifacts, :tags, status, user: user, pipeline: pipeline, stage: stage, stage_idx: stage_idx, name: name)
+      end
+
+      def create_bridge(stage, stage_idx, name, status)
+        create(:ci_bridge, status, pipeline: pipeline, stage: stage, stage_idx: stage_idx, name: name)
       end
 
       before do
-        create_build_with_artifacts('build', 0, 'job1')
-        create_build_with_artifacts('build', 0, 'job2')
+        create_build_with_artifacts('build', 0, 'job1', :failed)
+        create_build_with_artifacts('build', 0, 'job2', :running)
+        create_build_with_artifacts('build', 0, 'job3', :pending)
+        create_bridge('deploy', 1, 'deploy-a', :failed)
+        create_bridge('deploy', 1, 'deploy-b', :created)
       end
 
-      it 'avoids N+1 database queries', :request_store do
-        control_count = ActiveRecord::QueryRecorder.new { get_pipeline_html }.count
+      it 'avoids N+1 database queries', :request_store, :use_sql_query_cache do
+        # warm up
+        get_pipeline_html
         expect(response).to have_gitlab_http_status(:ok)
 
-        create_build_with_artifacts('build', 0, 'job3')
+        control = ActiveRecord::QueryRecorder.new(skip_cached: false) do
+          get_pipeline_html
+          expect(response).to have_gitlab_http_status(:ok)
+        end
+
+        create_build_with_artifacts('build', 0, 'job4', :failed)
+        create_build_with_artifacts('build', 0, 'job5', :running)
+        create_build_with_artifacts('build', 0, 'job6', :pending)
+        create_bridge('deploy', 1, 'deploy-c', :failed)
+        create_bridge('deploy', 1, 'deploy-d', :created)
 
-        expect { get_pipeline_html }.not_to exceed_query_limit(control_count)
-        expect(response).to have_gitlab_http_status(:ok)
+        expect do
+          get_pipeline_html
+          expect(response).to have_gitlab_http_status(:ok)
+        end.not_to exceed_all_query_limit(control)
       end
     end