Commit 1a9f1db4 authored by Erick Bajao's avatar Erick Bajao

Preload job artifacts archive in pipeline details

This makes the PipelineDetailsEntity to by default
eager load the job_artifacts_archive.

In the case of PipelineSerializer, it does not rely on
PipelineDetailsEntity for preloading, because it does its
own preloading of pipeline relations. This is more performant
for cases where in there are multiple pipelines to preload.
parent ef13bed5
......@@ -114,6 +114,7 @@ module Ci
end
scope :eager_load_job_artifacts, -> { includes(:job_artifacts) }
scope :eager_load_job_artifacts_archive, -> { includes(:job_artifacts_archive) }
scope :eager_load_everything, -> do
includes(
......
......@@ -8,7 +8,12 @@ class PipelineDetailsEntity < PipelineEntity
end
expose :details do
expose :artifacts, using: BuildArtifactEntity
expose :artifacts do |pipeline, options|
rel = pipeline.artifacts
rel = rel.eager_load_job_artifacts_archive if options.fetch(:preload_job_artifacts_archive, true)
BuildArtifactEntity.represent(rel, options)
end
expose :manual_actions, using: BuildActionEntity
expose :scheduled_actions, using: BuildActionEntity
end
......
......@@ -7,6 +7,10 @@ class PipelineSerializer < BaseSerializer
# rubocop: disable CodeReuse/ActiveRecord
def represent(resource, opts = {})
if resource.is_a?(ActiveRecord::Relation)
# We don't want PipelineDetailsEntity to preload the job_artifacts_archive
# because we do it with preloaded_relations in a more optimal way
# if the given resource is a collection of multiple pipelines.
opts[:preload_job_artifacts_archive] = false
resource = resource.preload(preloaded_relations)
end
......
---
title: Fix N+1 queries caused by loading job artifacts archive in pipeline details
entity
merge_request: 25250
author:
type: fixed
......@@ -171,7 +171,17 @@ describe Projects::PipelinesController do
def create_build(pipeline, stage, stage_idx, name, user = nil)
status = %w[created running pending success failed canceled].sample
create(:ci_build, pipeline: pipeline, stage: stage, stage_idx: stage_idx, name: name, status: status, user: user)
create(
:ci_build,
:artifacts,
artifacts_expire_at: 2.days.from_now,
pipeline: pipeline,
stage: stage,
stage_idx: stage_idx,
name: name,
status: status,
user: user
)
end
end
......
......@@ -173,5 +173,40 @@ describe PipelineDetailsEntity do
expect(subject[:triggered].first[:project]).not_to be_nil
end
end
context 'when pipeline has expiring archive artifacts' do
let(:pipeline) { create(:ci_empty_pipeline) }
let!(:build_1) { create(:ci_build, :artifacts, pipeline: pipeline, artifacts_expire_at: 2.days.from_now, name: 'build_1') }
let!(:build_2) { create(:ci_build, :artifacts, pipeline: pipeline, artifacts_expire_at: 2.days.from_now, name: 'build_2') }
let!(:build_3) { create(:ci_build, :artifacts, pipeline: pipeline, artifacts_expire_at: 2.days.from_now, name: 'build_3') }
let(:names) { subject[:details][:artifacts].map { |a| a[:name] } }
context 'and preload_job_artifacts_archive is not defined in the options' do
it 'defaults to true and eager loads the job_artifacts_archive' do
recorder = ActiveRecord::QueryRecorder.new do
expect(names).to match_array(%w[build_1 build_2 build_3])
end
# This makes only one query to fetch all job artifacts
expect(recorder.count).to eq(42)
end
end
context 'and preload_job_artifacts_archive is set to false' do
let(:entity) do
described_class.represent(pipeline, request: request, preload_job_artifacts_archive: false)
end
it 'does not eager load the job_artifacts_archive' do
recorder = ActiveRecord::QueryRecorder.new do
expect(names).to match_array(%w[build_1 build_2 build_3])
end
# This makes one query for each job artifact
expect(recorder.count).to eq(44)
end
end
end
end
end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment