Return artifacts, not builds

1. It was weird that a key named `artifacts` returned builds
2. We need BuildArtifactEntity to represent each artifact of a job in
    order for a job to have more than one downloadable artifact
3. We need BuildArtifactEntity to have access to an artifact's file type
    in order to properly create the download path

app/assets/javascripts/pipelines/components/pipelines_artifacts.vue

@@ -35,7 +35,7 @@ export default {
     <ul class="dropdown-menu dropdown-menu-right">
       <li v-for="(artifact, i) in artifacts" :key="i">
         <gl-link :href="artifact.path" rel="nofollow" download
-          >Download {{ artifact.name }} artifacts</gl-link
+          >Download {{ artifact.name }} artifact</gl-link
         >
       </li>
     </ul>

app/helpers/gitlab_routing_helper.rb

@@ -162,8 +162,8 @@ module GitlabRoutingHelper
   # against the arguments. We can speed this up 10x by generating the strings directly.
 
   # /*namespace_id/:project_id/-/jobs/:job_id/artifacts/download(.:format)
-  def fast_download_project_job_artifacts_path(project, job)
-    expose_fast_artifacts_path(project, job, :download)
+  def fast_download_project_job_artifacts_path(project, job, params = {})
+    expose_fast_artifacts_path(project, job, :download, params)
   end
 
   # /*namespace_id/:project_id/-/jobs/:job_id/artifacts/keep(.:format)
@@ -176,8 +176,13 @@ module GitlabRoutingHelper
     expose_fast_artifacts_path(project, job, :browse)
   end
 
-  def expose_fast_artifacts_path(project, job, action)
+  def expose_fast_artifacts_path(project, job, action, params = {})
     path = "#{project.full_path}/-/jobs/#{job.id}/artifacts/#{action}"
+
+    unless params.empty?
+      path += "?#{params.to_query}"
+    end
+
     Gitlab::Utils.append_path(Gitlab.config.gitlab.relative_url_root, path)
   end
 

app/models/ci/job_artifact.rb

@@ -112,6 +112,7 @@ module Ci
 
     after_save :update_file_store, if: :saved_change_to_file?
 
+    scope :not_expired, -> { where('expire_at IS NULL OR expire_at > ?', Time.current) }
     scope :with_files_stored_locally, -> { where(file_store: [nil, ::JobArtifactUploader::Store::LOCAL]) }
     scope :with_files_stored_remotely, -> { where(file_store: ::JobArtifactUploader::Store::REMOTE) }
     scope :for_sha, ->(sha, project_id) { joins(job: :pipeline).where(ci_pipelines: { sha: sha, project_id: project_id }) }
@@ -151,6 +152,7 @@ module Ci
     end
 
     scope :expired, -> (limit) { where('expire_at < ?', Time.current).limit(limit) }
+    scope :downloadable, -> { where(file_type: DOWNLOADABLE_TYPES) }
     scope :locked, -> { where(locked: true) }
     scope :unlocked, -> { where(locked: [false, nil]) }
 
@@ -246,6 +248,14 @@ module Ci
       super || self.file_location.nil?
     end
 
+    def expired?
+      expire_at.present? && expire_at < Time.current
+    end
+
+    def expiring?
+      expire_at.present? && expire_at > Time.current
+    end
+
     def expire_in
       expire_at - Time.current if expire_at
     end

app/models/ci/pipeline.rb

@@ -41,10 +41,13 @@ module Ci
     has_many :latest_statuses_ordered_by_stage, -> { latest.order(:stage_idx, :stage) }, class_name: 'CommitStatus', foreign_key: :commit_id, inverse_of: :pipeline
     has_many :processables, class_name: 'Ci::Processable', foreign_key: :commit_id, inverse_of: :pipeline
     has_many :builds, foreign_key: :commit_id, inverse_of: :pipeline
+    has_many :job_artifacts, through: :builds
     has_many :trigger_requests, dependent: :destroy, foreign_key: :commit_id # rubocop:disable Cop/ActiveRecordDependent
     has_many :variables, class_name: 'Ci::PipelineVariable'
     has_many :deployments, through: :builds
     has_many :environments, -> { distinct }, through: :deployments
+    has_many :latest_builds, -> { latest }, foreign_key: :commit_id, inverse_of: :pipeline, class_name: 'Ci::Build'
+    has_many :downloadable_artifacts, -> { not_expired.downloadable }, through: :latest_builds, source: :job_artifacts
 
     # Merge requests for which the current pipeline is running against
     # the merge request's latest commit.
@@ -56,7 +59,6 @@ module Ci
     has_many :cancelable_statuses, -> { cancelable }, foreign_key: :commit_id, class_name: 'CommitStatus'
     has_many :manual_actions, -> { latest.manual_actions.includes(:project) }, foreign_key: :commit_id, class_name: 'Ci::Build'
     has_many :scheduled_actions, -> { latest.scheduled_actions.includes(:project) }, foreign_key: :commit_id, class_name: 'Ci::Build'
-    has_many :artifacts, -> { latest.with_artifacts_not_expired.includes(:project) }, foreign_key: :commit_id, class_name: 'Ci::Build'
 
     has_many :auto_canceled_pipelines, class_name: 'Ci::Pipeline', foreign_key: 'auto_canceled_by_id'
     has_many :auto_canceled_jobs, class_name: 'CommitStatus', foreign_key: 'auto_canceled_by_id'

app/serializers/build_artifact_entity.rb

@@ -4,30 +4,28 @@ class BuildArtifactEntity < Grape::Entity
   include RequestAwareEntity
   include GitlabRoutingHelper
 
-  expose :name do |job|
-    job.name
-  end
-
-  expose :artifacts_expired?, as: :expired
-  expose :artifacts_expire_at, as: :expire_at
+  alias_method :artifact, :object
 
-  expose :path do |job|
-    fast_download_project_job_artifacts_path(project, job)
+  expose :name do |artifact|
+    "#{artifact.job.name}:#{artifact.file_type}"
   end
 
-  expose :keep_path, if: -> (*) { job.has_expiring_archive_artifacts? } do |job|
-    fast_keep_project_job_artifacts_path(project, job)
-  end
+  expose :expire_at
+  expose :expired?, as: :expired
 
-  expose :browse_path do |job|
-    fast_browse_project_job_artifacts_path(project, job)
+  expose :path do |artifact|
+    fast_download_project_job_artifacts_path(
+      artifact.project,
+      artifact.job,
+      file_type: artifact.file_type
+    )
   end
 
-  private
-
-  alias_method :job, :object
+  expose :keep_path, if: -> (*) { artifact.expiring? } do |artifact|
+    fast_keep_project_job_artifacts_path(artifact.project, artifact.job)
+  end
 
-  def project
-    job.project
+  expose :browse_path do |artifact|
+    fast_browse_project_job_artifacts_path(artifact.project, artifact.job)
   end
 end

app/serializers/pipeline_details_entity.rb

@@ -9,8 +9,7 @@ class PipelineDetailsEntity < PipelineEntity
 
   expose :details do
     expose :artifacts do |pipeline, options|
-      rel = pipeline.artifacts
-      rel = rel.eager_load_job_artifacts_archive if options.fetch(:preload_job_artifacts_archive, true)
+      rel = pipeline.downloadable_artifacts
 
       BuildArtifactEntity.represent(rel, options)
     end

app/serializers/pipeline_serializer.rb

@@ -7,10 +7,6 @@ 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
 
@@ -44,35 +40,29 @@ class PipelineSerializer < BaseSerializer
 
   def preloaded_relations
     [
-      :latest_statuses_ordered_by_stage,
-      :project,
-      :stages,
-      {
-        failed_builds: %i(project metadata)
-      },
-      :retryable_builds,
       :cancelable_statuses,
-      :trigger_requests,
+      :latest_statuses_ordered_by_stage,
       :manual_actions,
+      :retryable_builds,
       :scheduled_actions,
-      :artifacts,
+      :stages,
+      :trigger_requests,
       :user,
       {
+        downloadable_artifacts: {
+          project: [:route, { namespace: :route }],
+          job: []
+        },
+        failed_builds: %i(project metadata),
         merge_request: {
           source_project: [:route, { namespace: :route }],
           target_project: [:route, { namespace: :route }]
-        }
         },
-      {
         pending_builds: :project,
         project: [:route, { namespace: :route }],
-        artifacts: {
-          project: [:route, { namespace: :route }],
-          job_artifacts_archive: []
+        triggered_by_pipeline: [:project, :user],
+        triggered_pipelines: [:project, :user]
       }
-      },
-      { triggered_by_pipeline: [:project, :user] },
-      { triggered_pipelines: [:project, :user] }
     ]
   end
 end

changelogs/unreleased/218582-fix-artifact-downloads-without-new-route.yml

+---
+title: Fix 404 when downloading a non-archive artifact
+merge_request: 32811
+author:
+type: fixed

ee/app/models/ee/ci/job_artifact.rb

@@ -20,7 +20,6 @@ module EE
       SECRET_DETECTION_REPORT_TYPES = %w[secret_detection].freeze
       DAST_REPORT_TYPES = %w[dast].freeze
 
-      scope :not_expired, -> { where('expire_at IS NULL OR expire_at > ?', Time.current) }
       scope :project_id_in, ->(ids) { where(project_id: ids) }
       scope :with_files_stored_remotely, -> { where(file_store: ::JobArtifactUploader::Store::REMOTE) }
 

ee/app/models/ee/ci/pipeline.rb

@@ -14,7 +14,6 @@ module EE
       prepended do
         include UsageStatistics
 
-        has_many :job_artifacts, through: :builds
         has_many :vulnerabilities_occurrence_pipelines, class_name: 'Vulnerabilities::OccurrencePipeline'
         has_many :vulnerability_findings, source: :occurrence, through: :vulnerabilities_occurrence_pipelines, class_name: 'Vulnerabilities::Occurrence'
 
@@ -29,7 +28,7 @@ module EE
 
         # Legacy way to fetch security reports based on job name. This has been replaced by the reports feature.
         scope :with_legacy_security_reports, -> do
-          joins(:artifacts).where(ci_builds: { name: %w[sast secret_detection dependency_scanning sast:container container_scanning dast] })
+          joins(:downloadable_artifacts).where(ci_builds: { name: %w[sast secret_detection dependency_scanning sast:container container_scanning dast] })
         end
 
         scope :with_vulnerabilities, -> do
@@ -199,10 +198,6 @@ module EE
         feature_names = REPORT_LICENSED_FEATURES.fetch(file_type)
         feature_names.nil? || feature_names.any? { |feature| project.feature_available?(feature) }
       end
-
-      def artifacts_with_files
-        @artifacts_with_files ||= artifacts.includes(:job_artifacts_metadata, :job_artifacts_archive).to_a
-      end
     end
   end
 end

ee/spec/models/ci/pipeline_spec.rb

@@ -14,7 +14,6 @@ RSpec.describe Ci::Pipeline do
 
   it { is_expected.to have_many(:security_scans).through(:builds).class_name('Security::Scan') }
   it { is_expected.to have_many(:downstream_bridges) }
-  it { is_expected.to have_many(:job_artifacts).through(:builds) }
   it { is_expected.to have_many(:vulnerability_findings).through(:vulnerabilities_occurrence_pipelines).class_name('Vulnerabilities::Occurrence') }
   it { is_expected.to have_many(:vulnerabilities_occurrence_pipelines).class_name('Vulnerabilities::OccurrencePipeline') }
 

spec/features/projects/pipelines/pipelines_spec.rb

@@ -453,10 +453,12 @@ RSpec.describe 'Pipelines', :js do
       context 'downloadable pipelines' do
         context 'with artifacts' do
           let!(:with_artifacts) do
-            create(:ci_build, :artifacts, :success,
+            build = create(:ci_build, :success,
               pipeline: pipeline,
               name: 'rspec tests',
               stage: 'test')
+
+            create(:ci_job_artifact, :codequality, job: build)
           end
 
           before do
@@ -470,7 +472,7 @@ RSpec.describe 'Pipelines', :js do
           it 'has artifacts download dropdown' do
             find('.js-pipeline-dropdown-download').click
 
-            expect(page).to have_link(with_artifacts.name)
+            expect(page).to have_link(with_artifacts.file_type)
           end
 
           it 'has download attribute on download links' do

spec/helpers/gitlab_routing_helper_spec.rb

@@ -121,6 +121,16 @@ describe GitlabRoutingHelper do
       it 'matches the Rails download path' do
         expect(fast_download_project_job_artifacts_path(project, job)).to eq(download_project_job_artifacts_path(project, job))
       end
+
+      context 'when given parameters' do
+        it 'adds them to the path' do
+          expect(
+            fast_download_project_job_artifacts_path(project, job, file_type: :dast)
+          ).to eq(
+            download_project_job_artifacts_path(project, job, file_type: :dast)
+          )
+        end
+      end
     end
 
     describe '#fast_keep_project_job_artifacts_path' do

spec/lib/gitlab/import_export/all_models.yml

@@ -195,7 +195,7 @@ ci_pipelines:
 - cancelable_statuses
 - manual_actions
 - scheduled_actions
-- artifacts
+- downloadable_artifacts
 - pipeline_schedule
 - merge_requests_as_head_pipeline
 - merge_request
@@ -220,6 +220,7 @@ ci_pipelines:
 - pipeline_config
 - security_scans
 - daily_build_group_report_results
+- latest_builds
 pipeline_variables:
 - pipeline
 stages:

spec/models/ci/job_artifact_spec.rb

@@ -23,6 +23,14 @@ describe Ci::JobArtifact do
     subject { build(:ci_job_artifact, :archive, size: 107464) }
   end
 
+  describe '.not_expired' do
+    it 'returns artifacts that have not expired' do
+      _expired_artifact = create(:ci_job_artifact, :expired)
+
+      expect(described_class.not_expired).to contain_exactly(artifact)
+    end
+  end
+
   describe '.with_reports' do
     let!(:artifact) { create(:ci_job_artifact, :archive) }
 
@@ -118,6 +126,17 @@ describe Ci::JobArtifact do
     end
   end
 
+  describe '.downloadable' do
+    subject { described_class.downloadable }
+
+    it 'filters for downloadable artifacts' do
+      downloadable_artifact = create(:ci_job_artifact, :codequality)
+      _not_downloadable_artifact = create(:ci_job_artifact, :trace)
+
+      expect(subject).to contain_exactly(downloadable_artifact)
+    end
+  end
+
   describe '.archived_trace_exists_for?' do
     subject { described_class.archived_trace_exists_for?(job_id) }
 
@@ -357,6 +376,62 @@ describe Ci::JobArtifact do
     end
   end
 
+  describe 'expired?' do
+    subject { artifact.expired? }
+
+    context 'when expire_at is nil' do
+      let(:artifact) { build(:ci_job_artifact, expire_at: nil) }
+
+      it 'returns false' do
+        is_expected.to be_falsy
+      end
+    end
+
+    context 'when expire_at is in the past' do
+      let(:artifact) { build(:ci_job_artifact, expire_at: Date.yesterday) }
+
+      it 'returns true' do
+        is_expected.to be_truthy
+      end
+    end
+
+    context 'when expire_at is in the future' do
+      let(:artifact) { build(:ci_job_artifact, expire_at: Date.tomorrow) }
+
+      it 'returns false' do
+        is_expected.to be_falsey
+      end
+    end
+  end
+
+  describe '#expiring?' do
+    subject { artifact.expiring? }
+
+    context 'when expire_at is nil' do
+      let(:artifact) { build(:ci_job_artifact, expire_at: nil) }
+
+      it 'returns false' do
+        is_expected.to be_falsy
+      end
+    end
+
+    context 'when expire_at is in the past' do
+      let(:artifact) { build(:ci_job_artifact, expire_at: Date.yesterday) }
+
+      it 'returns false' do
+        is_expected.to be_falsy
+      end
+    end
+
+    context 'when expire_at is in the future' do
+      let(:artifact) { build(:ci_job_artifact, expire_at: Date.tomorrow) }
+
+      it 'returns true' do
+        is_expected.to be_truthy
+      end
+    end
+  end
+
   describe '#expire_in' do
     subject { artifact.expire_in }
 

spec/models/ci/pipeline_spec.rb

@@ -26,6 +26,7 @@ describe Ci::Pipeline, :mailer do
   it { is_expected.to have_many(:trigger_requests) }
   it { is_expected.to have_many(:variables) }
   it { is_expected.to have_many(:builds) }
+  it { is_expected.to have_many(:job_artifacts).through(:builds) }
   it { is_expected.to have_many(:auto_canceled_pipelines) }
   it { is_expected.to have_many(:auto_canceled_jobs) }
   it { is_expected.to have_many(:sourced_pipelines) }
@@ -51,6 +52,27 @@ describe Ci::Pipeline, :mailer do
       expect(Project.reflect_on_association(:all_pipelines).has_inverse?).to eq(:project)
       expect(Project.reflect_on_association(:ci_pipelines).has_inverse?).to eq(:project)
     end
+
+    describe '#latest_builds' do
+      it 'has a one to many relationship with its latest builds' do
+        _old_build = create(:ci_build, :retried, pipeline: pipeline)
+        latest_build = create(:ci_build, :expired, pipeline: pipeline)
+
+        expect(pipeline.latest_builds).to contain_exactly(latest_build)
+      end
+    end
+
+    describe '#downloadable_artifacts' do
+      let(:build) { create(:ci_build, pipeline: pipeline) }
+
+      it 'returns downloadable artifacts that have not expired' do
+        downloadable_artifact = create(:ci_job_artifact, :codequality, job: build)
+        _expired_artifact = create(:ci_job_artifact, :junit, :expired, job: build)
+        _undownloadable_artifact = create(:ci_job_artifact, :trace, job: build)
+
+        expect(pipeline.downloadable_artifacts).to contain_exactly(downloadable_artifact)
+      end
+    end
   end
 
   describe '#set_status' do

spec/serializers/build_artifact_entity_spec.rb

@@ -3,17 +3,18 @@
 require 'spec_helper'
 
 describe BuildArtifactEntity do
-  let(:job) { create(:ci_build, :artifacts, name: 'test:job', artifacts_expire_at: 1.hour.from_now) }
+  let(:job) { create(:ci_build) }
+  let(:artifact) { create(:ci_job_artifact, :codequality, expire_at: 1.hour.from_now, job: job) }
 
   let(:entity) do
-    described_class.new(job, request: double)
+    described_class.new(artifact, request: double)
   end
 
   describe '#as_json' do
     subject { entity.as_json }
 
     it 'contains job name' do
-      expect(subject[:name]).to eq 'test:job'
+      expect(subject[:name]).to eq "test:codequality"
     end
 
     it 'exposes information about expiration of artifacts' do
@@ -22,7 +23,7 @@ describe BuildArtifactEntity do
 
     it 'contains paths to the artifacts' do
       expect(subject[:path])
-        .to include "jobs/#{job.id}/artifacts/download"
+        .to include "jobs/#{job.id}/artifacts/download?file_type=codequality"
 
       expect(subject[:keep_path])
         .to include "jobs/#{job.id}/artifacts/keep"

spec/serializers/pipeline_details_entity_spec.rb

@@ -173,44 +173,5 @@ 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
-
-          expected_queries = Gitlab.ee? ? 42 : 29
-
-          # This makes only one query to fetch all job artifacts
-          expect(recorder.count).to eq(expected_queries)
-        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
-
-          expected_queries = Gitlab.ee? ? 44 : 31
-
-          # This makes one query for each job artifact
-          expect(recorder.count).to eq(expected_queries)
-        end
-      end
-    end
   end
 end