diff --git a/app/presenters/ci/build_presenter.rb b/app/presenters/ci/build_presenter.rb index 384cb3285fc0c34cf68971a5e8fb5c6efd8e2741..06ed6791bb745bfe62f7098eac9f7b46e6e48822 100644 --- a/app/presenters/ci/build_presenter.rb +++ b/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 diff --git a/app/presenters/ci/stage_presenter.rb b/app/presenters/ci/stage_presenter.rb index 9ec3f8d153a1598d0a0c04a01084f5e6090bcd81..2d5ee3d2f25e8730076efff2c12577561c5ba0e2 100644 --- a/app/presenters/ci/stage_presenter.rb +++ b/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 diff --git a/app/views/projects/ci/builds/_build.html.haml b/app/views/projects/ci/builds/_build.html.haml index 1a3813ba99fd9ad55902c577ded6750dd31d677d..437529c3608972d3144821b0203f63b7ed6e5808 100644 --- a/app/views/projects/ci/builds/_build.html.haml +++ b/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) diff --git a/spec/controllers/projects/pipelines_controller_spec.rb b/spec/controllers/projects/pipelines_controller_spec.rb index 65a563fac7ce7ff3385843d0aad2c0245457ef4f..774971e992d7f398ac2d96b897292e0cd47437ce 100644 --- a/spec/controllers/projects/pipelines_controller_spec.rb +++ b/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