Merge branch 'sh-fix-nplus-one-pipelines-show' into 'master'

Fix N+1 SQL queries in  PipelinesController#show

See merge request gitlab-org/gitlab!60794

app/controllers/projects/pipelines_controller.rb

@@ -227,7 +227,7 @@ class Projects::PipelinesController < Projects::ApplicationController
   end
 
   def render_show
-    @stages = @pipeline.stages.with_latest_and_retried_statuses
+    @stages = @pipeline.stages
 
     respond_to do |format|
       format.html do

app/models/ci/stage.rb

@@ -6,6 +6,7 @@ module Ci
     include Importable
     include Ci::HasStatus
     include Gitlab::OptimisticLocking
+    include Presentable
 
     enum status: Ci::HasStatus::STATUSES_ENUM
 
@@ -22,12 +23,6 @@ module Ci
     scope :ordered, -> { order(position: :asc) }
     scope :in_pipelines, ->(pipelines) { where(pipeline: pipelines) }
     scope :by_name, ->(names) { where(name: names) }
-    scope :with_latest_and_retried_statuses, -> do
-      includes(
-        latest_statuses: [:pipeline, project: :namespace],
-        retried_statuses: [:pipeline, project: :namespace]
-      )
-    end
 
     with_options unless: :importing? do
       validates :project, presence: true

app/presenters/ci/stage_presenter.rb

+# frozen_string_literal: true
+
+module Ci
+  class StagePresenter < Gitlab::View::Presenter::Delegated
+    presents :stage
+
+    def latest_ordered_statuses
+      preload_statuses(stage.statuses.latest_ordered)
+    end
+
+    def retried_ordered_statuses
+      preload_statuses(stage.statuses.retried_ordered)
+    end
+
+    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
+
+    def preloadable_statuses(statuses)
+      statuses.reject do |status|
+        status.instance_of?(::GenericCommitStatus) || status.instance_of?(::Ci::Bridge)
+      end
+    end
+  end
+end

app/views/projects/stage/_stage.html.haml

+- stage = stage.present(current_user: current_user)
+
 %tr
   %th{ colspan: 10 }
     %strong
@@ -6,8 +8,8 @@
         = ci_icon_for_status(stage.status)
        
       = stage.name.titleize
-= render stage.latest_statuses, stage: false, ref: false, pipeline_link: false, allow_retry: true
-= render stage.retried_statuses, stage: false, ref: false, pipeline_link: false, retried: true
+= render stage.latest_ordered_statuses, stage: false, ref: false, pipeline_link: false, allow_retry: true
+= render stage.retried_ordered_statuses, stage: false, ref: false, pipeline_link: false, retried: true
 %tr
   %td{ colspan: 10 }
      

changelogs/unreleased/sh-fix-nplus-one-pipelines-show.yml

+---
+title: Fix N+1 SQL queries in PipelinesController#show
+merge_request: 60794
+author:
+type: fixed

spec/controllers/projects/pipelines_controller_spec.rb

@@ -290,6 +290,39 @@ RSpec.describe Projects::PipelinesController do
     end
   end
 
+  describe 'GET #show' do
+    render_views
+
+    let_it_be(:pipeline) { create(:ci_pipeline, project: project) }
+
+    subject { get_pipeline_html }
+
+    def get_pipeline_html
+      get :show, params: { namespace_id: project.namespace, project_id: project, id: pipeline }, format: :html
+    end
+
+    def create_build_with_artifacts(stage, stage_idx, name)
+      create(:ci_build, :artifacts, :tags, 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')
+    end
+
+    it 'avoids N+1 database queries', :request_store do
+      get_pipeline_html
+
+      control_count = ActiveRecord::QueryRecorder.new { get_pipeline_html }.count
+      expect(response).to have_gitlab_http_status(:ok)
+
+      create_build_with_artifacts('build', 0, 'job3')
+
+      expect { get_pipeline_html }.not_to exceed_query_limit(control_count)
+      expect(response).to have_gitlab_http_status(:ok)
+    end
+  end
+
   describe 'GET show.json' do
     let(:pipeline) { create(:ci_pipeline, project: project) }
 

spec/presenters/ci/stage_presenter_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Ci::StagePresenter do
+  let(:stage) { create(:ci_stage) }
+  let(:presenter) { described_class.new(stage) }
+
+  let!(:build) { create(:ci_build, :tags, :artifacts, pipeline: stage.pipeline, stage: stage.name) }
+  let!(:retried_build) { create(:ci_build, :tags, :artifacts, :retried, pipeline: stage.pipeline, stage: stage.name) }
+
+  before do
+    create(:generic_commit_status, pipeline: stage.pipeline, stage: stage.name)
+  end
+
+  shared_examples 'preloaded associations for CI status' do
+    it 'preloads project' do
+      expect(presented_stage.association(:project)).to be_loaded
+    end
+
+    it 'preloads build pipeline' do
+      expect(presented_stage.association(:pipeline)).to be_loaded
+    end
+
+    it 'preloads build tags' do
+      expect(presented_stage.association(:tags)).to be_loaded
+    end
+
+    it 'preloads build artifacts archive' do
+      expect(presented_stage.association(:job_artifacts_archive)).to be_loaded
+    end
+
+    it 'preloads build artifacts metadata' do
+      expect(presented_stage.association(:metadata)).to be_loaded
+    end
+  end
+
+  describe '#latest_ordered_statuses' do
+    subject(:presented_stage) { presenter.latest_ordered_statuses.second }
+
+    it_behaves_like 'preloaded associations for CI status'
+  end
+
+  describe '#retried_ordered_statuses' do
+    subject(:presented_stage) { presenter.retried_ordered_statuses.first }
+
+    it_behaves_like 'preloaded associations for CI status'
+  end
+end