Merge branch 'refactor-artifact-api-endpoint' into 'master'

Refactor artifact api endpoint

Closes #55445

See merge request gitlab-org/gitlab-ce!23582

5 files changed, 122 insertions, 83 deletions

diff --git a/app/controllers/projects/artifacts_controller.rb b/app/controllers/projects/artifacts_controller.rb
index 1a91e07b97f..9a0997e92ee 100644
--- a/app/controllers/projects/artifacts_controller.rb
+++ b/app/controllers/projects/artifacts_controller.rb
@@ -85,20 +85,15 @@ class Projects::ArtifactsController < Projects::ApplicationController
     end
   end
 
-  # rubocop: disable CodeReuse/ActiveRecord
   def build_from_id
-    project.builds.find_by(id: params[:job_id]) if params[:job_id]
+    project.get_build(params[:job_id]) if params[:job_id]
   end
-  # rubocop: enable CodeReuse/ActiveRecord
 
-  # rubocop: disable CodeReuse/ActiveRecord
   def build_from_ref
     return unless @ref_name
 
-    builds = project.latest_successful_builds_for(@ref_name)
-    builds.find_by(name: params[:job])
+    project.latest_successful_build_for(params[:job], @ref_name)
   end
-  # rubocop: enable CodeReuse/ActiveRecord
 
   def artifacts_file
     @artifacts_file ||= build&.artifacts_file_for_type(params[:file_type] || :archive)
diff --git a/app/controllers/projects/build_artifacts_controller.rb b/app/controllers/projects/build_artifacts_controller.rb
index 7d4d566499c..d3d5ba5c75d 100644
--- a/app/controllers/projects/build_artifacts_controller.rb
+++ b/app/controllers/projects/build_artifacts_controller.rb
@@ -44,18 +44,13 @@ class Projects::BuildArtifactsController < Projects::ApplicationController
     @job ||= job_from_id || job_from_ref
   end
 
-  # rubocop: disable CodeReuse/ActiveRecord
   def job_from_id
-    project.builds.find_by(id: params[:build_id]) if params[:build_id]
+    project.get_build(params[:build_id]) if params[:build_id]
   end
-  # rubocop: enable CodeReuse/ActiveRecord
 
-  # rubocop: disable CodeReuse/ActiveRecord
   def job_from_ref
     return unless @ref_name
 
-    jobs = project.latest_successful_builds_for(@ref_name)
-    jobs.find_by(name: params[:job])
+    project.latest_successful_build_for(params[:job], @ref_name)
   end
-  # rubocop: enable CodeReuse/ActiveRecord
 end
diff --git a/app/models/project.rb b/app/models/project.rb
index cab173503ce..a66ed6736ca 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -647,19 +647,19 @@ class Project < ActiveRecord::Base
   end
 
   # ref can't be HEAD, can only be branch/tag name or SHA
-  def latest_successful_builds_for(ref = default_branch)
+  def latest_successful_build_for(job_name, ref = default_branch)
     latest_pipeline = ci_pipelines.latest_successful_for(ref)
+    return unless latest_pipeline
 
-    if latest_pipeline
-      latest_pipeline.builds.latest.with_artifacts_archive
-    else
-      builds.none
-    end
+    latest_pipeline.builds.latest.with_artifacts_archive.find_by(name: job_name)
   end
 
-  def latest_successful_build_for(job_name, ref = default_branch)
-    builds = latest_successful_builds_for(ref)
-    builds.find_by!(name: job_name)
+  def latest_successful_build_for!(job_name, ref = default_branch)
+    latest_successful_build_for(job_name, ref) || raise(ActiveRecord::RecordNotFound.new("Couldn't find job #{job_name}"))
+  end
+
+  def get_build(id)
+    builds.find_by(id: id)
   end
 
   def merge_base_commit(first_commit_id, second_commit_id)
diff --git a/lib/api/job_artifacts.rb b/lib/api/job_artifacts.rb
index a4068a200b3..933bd067e26 100644
--- a/lib/api/job_artifacts.rb
+++ b/lib/api/job_artifacts.rb
@@ -23,17 +23,14 @@ module API
         requires :job,      type: String, desc: 'The name for the job'
       end
       route_setting :authentication, job_token_allowed: true
-      # rubocop: disable CodeReuse/ActiveRecord
       get ':id/jobs/artifacts/:ref_name/download',
         requirements: { ref_name: /.+/ } do
         authorize_download_artifacts!
 
-        builds = user_project.latest_successful_builds_for(params[:ref_name])
-        latest_build = builds.find_by!(name: params[:job])
+        latest_build = user_project.latest_successful_build_for!(params[:job], params[:ref_name])
 
         present_carrierwave_file!(latest_build.artifacts_file)
       end
-      # rubocop: enable CodeReuse/ActiveRecord
 
       desc 'Download a specific file from artifacts archive from a ref' do
         detail 'This feature was introduced in GitLab 11.5'
@@ -48,7 +45,7 @@ module API
           requirements: { ref_name: /.+/ } do
         authorize_download_artifacts!
 
-        build = user_project.latest_successful_build_for(params[:job], params[:ref_name])
+        build = user_project.latest_successful_build_for!(params[:job], params[:ref_name])
 
         path = Gitlab::Ci::Build::Artifacts::Path
                  .new(params[:artifact_path])
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index d1ab0bdba29..3044150bca8 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -1105,13 +1105,13 @@ describe Project do
 
   describe '#pipeline_for' do
     let(:project) { create(:project, :repository) }
-    let!(:pipeline) { create_pipeline }
+    let!(:pipeline) { create_pipeline(project) }
 
     shared_examples 'giving the correct pipeline' do
       it { is_expected.to eq(pipeline) }
 
       context 'return latest' do
-        let!(:pipeline2) { create_pipeline }
+        let!(:pipeline2) { create_pipeline(project) }
 
         it { is_expected.to eq(pipeline2) }
       end
@@ -1128,13 +1128,6 @@ describe Project do
 
       it_behaves_like 'giving the correct pipeline'
     end
-
-    def create_pipeline
-      create(:ci_pipeline,
-             project: project,
-             ref: 'master',
-             sha: project.commit('master').sha)
-    end
   end
 
   describe '#builds_enabled' do
@@ -1922,38 +1915,21 @@ describe Project do
     end
   end
 
-  describe '#latest_successful_builds_for and #latest_successful_build_for' do
-    def create_pipeline(status = 'success')
-      create(:ci_pipeline, project: project,
-                           sha: project.commit.sha,
-                           ref: project.default_branch,
-                           status: status)
-    end
-
-    def create_build(new_pipeline = pipeline, name = 'test')
-      create(:ci_build, :success, :artifacts,
-             pipeline: new_pipeline,
-             status: new_pipeline.status,
-             name: name)
-    end
-
+  describe '#latest_successful_build_for' do
     let(:project) { create(:project, :repository) }
-    let(:pipeline) { create_pipeline }
+    let(:pipeline) { create_pipeline(project) }
 
     context 'with many builds' do
       it 'gives the latest builds from latest pipeline' do
-        pipeline1 = create_pipeline
-        pipeline2 = create_pipeline
+        pipeline1 = create_pipeline(project)
+        pipeline2 = create_pipeline(project)
         create_build(pipeline1, 'test')
         create_build(pipeline1, 'test2')
         build1_p2 = create_build(pipeline2, 'test')
-        build2_p2 = create_build(pipeline2, 'test2')
+        create_build(pipeline2, 'test2')
 
-        latest_builds = project.latest_successful_builds_for
-        single_build = project.latest_successful_build_for(build1_p2.name)
-
-        expect(latest_builds).to contain_exactly(build2_p2, build1_p2)
-        expect(single_build).to eq(build1_p2)
+        expect(project.latest_successful_build_for(build1_p2.name))
+          .to eq(build1_p2)
       end
     end
 
@@ -1962,53 +1938,115 @@ describe Project do
 
       context 'standalone pipeline' do
         it 'returns builds for ref for default_branch' do
-          builds = project.latest_successful_builds_for
-          single_build = project.latest_successful_build_for(build.name)
+          expect(project.latest_successful_build_for(build.name))
+            .to eq(build)
+        end
 
-          expect(builds).to contain_exactly(build)
-          expect(single_build).to eq(build)
+        it 'returns empty relation if the build cannot be found' do
+          expect(project.latest_successful_build_for('TAIL'))
+            .to be_nil
         end
+      end
 
-        it 'returns empty relation if the build cannot be found for #latest_successful_builds_for' do
-          builds = project.latest_successful_builds_for('TAIL')
+      context 'with some pending pipeline' do
+        before do
+          create_build(create_pipeline(project, 'pending'))
+        end
 
-          expect(builds).to be_kind_of(ActiveRecord::Relation)
-          expect(builds).to be_empty
+        it 'gives the latest build from latest pipeline' do
+          expect(project.latest_successful_build_for(build.name))
+            .to eq(build)
         end
+      end
+    end
 
-        it 'returns exception if the build cannot be found for #latest_successful_build_for' do
-          expect { project.latest_successful_build_for(build.name, 'TAIL') }.to raise_error(ActiveRecord::RecordNotFound)
+    context 'with pending pipeline' do
+      it 'returns empty relation' do
+        pipeline.update(status: 'pending')
+        pending_build = create_build(pipeline)
+
+        expect(project.latest_successful_build_for(pending_build.name)).to be_nil
+      end
+    end
+  end
+
+  describe '#latest_successful_build_for!' do
+    let(:project) { create(:project, :repository) }
+    let(:pipeline) { create_pipeline(project) }
+
+    context 'with many builds' do
+      it 'gives the latest builds from latest pipeline' do
+        pipeline1 = create_pipeline(project)
+        pipeline2 = create_pipeline(project)
+        create_build(pipeline1, 'test')
+        create_build(pipeline1, 'test2')
+        build1_p2 = create_build(pipeline2, 'test')
+        create_build(pipeline2, 'test2')
+
+        expect(project.latest_successful_build_for(build1_p2.name))
+          .to eq(build1_p2)
+      end
+    end
+
+    context 'with succeeded pipeline' do
+      let!(:build) { create_build }
+
+      context 'standalone pipeline' do
+        it 'returns builds for ref for default_branch' do
+          expect(project.latest_successful_build_for!(build.name))
+            .to eq(build)
+        end
+
+        it 'returns exception if the build cannot be found' do
+          expect { project.latest_successful_build_for!(build.name, 'TAIL') }
+            .to raise_error(ActiveRecord::RecordNotFound)
         end
       end
 
       context 'with some pending pipeline' do
         before do
-          create_build(create_pipeline('pending'))
+          create_build(create_pipeline(project, 'pending'))
         end
 
         it 'gives the latest build from latest pipeline' do
-          latest_builds = project.latest_successful_builds_for
-          last_single_build = project.latest_successful_build_for(build.name)
-
-          expect(latest_builds).to contain_exactly(build)
-          expect(last_single_build).to eq(build)
+          expect(project.latest_successful_build_for!(build.name))
+            .to eq(build)
         end
       end
     end
 
     context 'with pending pipeline' do
-      before do
+      it 'returns empty relation' do
         pipeline.update(status: 'pending')
-        create_build(pipeline)
+        pending_build = create_build(pipeline)
+
+        expect { project.latest_successful_build_for!(pending_build.name) }
+          .to raise_error(ActiveRecord::RecordNotFound)
       end
+    end
+  end
 
-      it 'returns empty relation' do
-        builds = project.latest_successful_builds_for
+  describe '#get_build' do
+    let(:project) { create(:project, :repository) }
+    let(:ci_pipeline) { create(:ci_pipeline, project: project) }
+
+    context 'when build exists' do
+      context 'build is associated with project' do
+        let(:build) { create(:ci_build, :success, pipeline: ci_pipeline) }
+
+        it { expect(project.get_build(build.id)).to eq(build) }
+      end
 
-        expect(builds).to be_kind_of(ActiveRecord::Relation)
-        expect(builds).to be_empty
+      context 'build is not associated with project' do
+        let(:build) { create(:ci_build, :success) }
+
+        it { expect(project.get_build(build.id)).to be_nil }
       end
     end
+
+    context 'build does not exists' do
+      it { expect(project.get_build(rand 100)).to be_nil }
+    end
   end
 
   describe '#import_status' do
@@ -4390,4 +4428,18 @@ describe Project do
   def rugged_config
     rugged_repo(project.repository).config
   end
+
+  def create_pipeline(project, status = 'success')
+    create(:ci_pipeline, project: project,
+                         sha: project.commit.sha,
+                         ref: project.default_branch,
+                         status: status)
+  end
+
+  def create_build(new_pipeline = pipeline, name = 'test')
+    create(:ci_build, :success, :artifacts,
+           pipeline: new_pipeline,
+           status: new_pipeline.status,
+           name: name)
+  end
 end