Commit a903d6c7 authored by Maxime Orefice's avatar Maxime Orefice

Remove artifacts from pipeline details entity

This commit removes the artifacts attribute from pipeline details
entity. We are now lazy loading all artifacts when a user request
them instead of loading everything on page load.

Changelog: performance
parent 16abe648
...@@ -223,7 +223,7 @@ class Projects::PipelinesController < Projects::ApplicationController ...@@ -223,7 +223,7 @@ class Projects::PipelinesController < Projects::ApplicationController
PipelineSerializer PipelineSerializer
.new(project: @project, current_user: @current_user) .new(project: @project, current_user: @current_user)
.with_pagination(request, response) .with_pagination(request, response)
.represent(@pipelines, disable_coverage: true, preload: true, disable_artifacts: true) .represent(@pipelines, disable_coverage: true, preload: true)
end end
def render_show def render_show
......
...@@ -8,15 +8,6 @@ class PipelineDetailsEntity < Ci::PipelineEntity ...@@ -8,15 +8,6 @@ class PipelineDetailsEntity < Ci::PipelineEntity
end end
expose :details do expose :details do
expose :artifacts, unless: proc { options[:disable_artifacts] } do |pipeline, options|
rel = pipeline.downloadable_artifacts
if Feature.enabled?(:non_public_artifacts, type: :development)
rel = rel.select { |artifact| can?(request.current_user, :read_job_artifacts, artifact.job) }
end
BuildArtifactEntity.represent(rel, options.merge(project: pipeline.project))
end
expose :manual_actions, using: BuildActionEntity expose :manual_actions, using: BuildActionEntity
expose :scheduled_actions, using: BuildActionEntity expose :scheduled_actions, using: BuildActionEntity
end end
......
...@@ -49,10 +49,6 @@ class PipelineSerializer < BaseSerializer ...@@ -49,10 +49,6 @@ class PipelineSerializer < BaseSerializer
{ {
manual_actions: :metadata, manual_actions: :metadata,
scheduled_actions: :metadata, scheduled_actions: :metadata,
downloadable_artifacts: {
project: [:route, { namespace: :route }],
job: []
},
failed_builds: %i(project metadata), failed_builds: %i(project metadata),
merge_request: { merge_request: {
source_project: [:route, { namespace: :route }], source_project: [:route, { namespace: :route }],
......
---
title: Stop exposing artifacts in pipelines.json
merge_request: 61253
author:
type: performance
...@@ -42,6 +42,4 @@ RSpec.describe MergeRequests::PipelineEntity do ...@@ -42,6 +42,4 @@ RSpec.describe MergeRequests::PipelineEntity do
expect(entity.as_json).not_to include(:coverage) expect(entity.as_json).not_to include(:coverage)
end end
end end
it_behaves_like 'public artifacts'
end end
...@@ -32,7 +32,7 @@ RSpec.describe PipelineDetailsEntity do ...@@ -32,7 +32,7 @@ RSpec.describe PipelineDetailsEntity do
expect(subject[:details]) expect(subject[:details])
.to include :duration, :finished_at .to include :duration, :finished_at
expect(subject[:details]) expect(subject[:details])
.to include :stages, :artifacts, :manual_actions, :scheduled_actions .to include :stages, :manual_actions, :scheduled_actions
expect(subject[:details][:status]).to include :icon, :favicon, :text, :label expect(subject[:details][:status]).to include :icon, :favicon, :text, :label
end end
...@@ -184,7 +184,5 @@ RSpec.describe PipelineDetailsEntity do ...@@ -184,7 +184,5 @@ RSpec.describe PipelineDetailsEntity do
expect(source_jobs[child_pipeline.id][:name]).to eq('child') expect(source_jobs[child_pipeline.id][:name]).to eq('child')
end end
end end
it_behaves_like 'public artifacts'
end end
end end
...@@ -155,7 +155,7 @@ RSpec.describe PipelineSerializer do ...@@ -155,7 +155,7 @@ RSpec.describe PipelineSerializer do
it 'verifies number of queries', :request_store do it 'verifies number of queries', :request_store do
recorded = ActiveRecord::QueryRecorder.new { subject } recorded = ActiveRecord::QueryRecorder.new { subject }
expected_queries = Gitlab.ee? ? 39 : 36 expected_queries = Gitlab.ee? ? 33 : 30
expect(recorded.count).to be_within(2).of(expected_queries) expect(recorded.count).to be_within(2).of(expected_queries)
expect(recorded.cached_count).to eq(0) expect(recorded.cached_count).to eq(0)
...@@ -176,7 +176,7 @@ RSpec.describe PipelineSerializer do ...@@ -176,7 +176,7 @@ RSpec.describe PipelineSerializer do
# pipeline. With the same ref this check is cached but if refs are # pipeline. With the same ref this check is cached but if refs are
# different then there is an extra query per ref # different then there is an extra query per ref
# https://gitlab.com/gitlab-org/gitlab-foss/issues/46368 # https://gitlab.com/gitlab-org/gitlab-foss/issues/46368
expected_queries = Gitlab.ee? ? 42 : 39 expected_queries = Gitlab.ee? ? 36 : 33
expect(recorded.count).to be_within(2).of(expected_queries) expect(recorded.count).to be_within(2).of(expected_queries)
expect(recorded.cached_count).to eq(0) expect(recorded.cached_count).to eq(0)
...@@ -202,7 +202,7 @@ RSpec.describe PipelineSerializer do ...@@ -202,7 +202,7 @@ RSpec.describe PipelineSerializer do
# Existing numbers are high and require performance optimization # Existing numbers are high and require performance optimization
# Ongoing issue: # Ongoing issue:
# https://gitlab.com/gitlab-org/gitlab/-/issues/225156 # https://gitlab.com/gitlab-org/gitlab/-/issues/225156
expected_queries = Gitlab.ee? ? 82 : 76 expected_queries = Gitlab.ee? ? 77 : 70
expect(recorded.count).to be_within(2).of(expected_queries) expect(recorded.count).to be_within(2).of(expected_queries)
expect(recorded.cached_count).to eq(0) expect(recorded.cached_count).to eq(0)
...@@ -221,8 +221,7 @@ RSpec.describe PipelineSerializer do ...@@ -221,8 +221,7 @@ RSpec.describe PipelineSerializer do
create(:ci_build, :scheduled, project: project, environment: env.name) create(:ci_build, :scheduled, project: project, environment: env.name)
recorded = ActiveRecord::QueryRecorder.new { subject } recorded = ActiveRecord::QueryRecorder.new { subject }
expected_queries = Gitlab.ee? ? 61 : 57 expected_queries = Gitlab.ee? ? 56 : 52
expect(recorded.count).to be_within(1).of(expected_queries) expect(recorded.count).to be_within(1).of(expected_queries)
expect(recorded.cached_count).to eq(0) expect(recorded.cached_count).to eq(0)
end end
......
# frozen_string_literal: true
RSpec.shared_examples 'public artifacts' do
let_it_be(:project) { create(:project, :public) }
let(:pipeline) { create(:ci_empty_pipeline, status: :success, project: project) }
context 'that has artifacts' do
let!(:build) { create(:ci_build, :success, :artifacts, pipeline: pipeline) }
it 'contains information about artifacts' do
expect(subject[:details][:artifacts].length).to eq(1)
end
end
context 'that has non public artifacts' do
let!(:build) { create(:ci_build, :success, :artifacts, :non_public_artifacts, pipeline: pipeline) }
it 'does not contain information about artifacts' do
expect(subject[:details][:artifacts].length).to eq(0)
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