Merge branch 'gracefully-handle-duplicate-artifacts-uploads' into 'master'

Gracefully handle duplicate artifacts uploads

See merge request gitlab-org/gitlab!24165

app/services/ci/create_job_artifacts_service.rb

+# frozen_string_literal: true
+
+module Ci
+  class CreateJobArtifactsService
+    ArtifactsExistError = Class.new(StandardError)
+
+    def execute(job, artifacts_file, params, metadata_file: nil)
+      expire_in = params['expire_in'] ||
+        Gitlab::CurrentSettings.current_application_settings.default_artifacts_expire_in
+
+      job.job_artifacts.build(
+        project: job.project,
+        file: artifacts_file,
+        file_type: params['artifact_type'],
+        file_format: params['artifact_format'],
+        file_sha256: artifacts_file.sha256,
+        expire_in: expire_in)
+
+      if metadata_file
+        job.job_artifacts.build(
+          project: job.project,
+          file: metadata_file,
+          file_type: :metadata,
+          file_format: :gzip,
+          file_sha256: metadata_file.sha256,
+          expire_in: expire_in)
+      end
+
+      job.update(artifacts_expire_in: expire_in)
+    rescue ActiveRecord::RecordNotUnique => error
+      return true if sha256_matches_existing_artifact?(job, params['artifact_type'], artifacts_file)
+
+      Gitlab::ErrorTracking.track_exception(error,
+        job_id: job.id,
+        project_id: job.project_id,
+        uploading_type: params['artifact_type']
+      )
+
+      job.errors.add(:base, 'another artifact of the same type already exists')
+      false
+    end
+
+    private
+
+    def sha256_matches_existing_artifact?(job, artifact_type, artifacts_file)
+      existing_artifact = job.job_artifacts.find_by_file_type(artifact_type)
+      return false unless existing_artifact
+
+      existing_artifact.file_sha256 == artifacts_file.sha256
+    end
+  end
+end

changelogs/unreleased/gracefully-handle-duplicate-artifacts-uploads.yml

+---
+title: Replace artifacts via Runner API if already exist
+merge_request: 24165
+author:
+type: fixed

lib/api/runner.rb

@@ -276,29 +276,8 @@ module API
         bad_request!('Missing artifacts file!') unless artifacts
         file_too_large! unless artifacts.size < max_artifacts_size(job)
 
-        expire_in = params['expire_in'] ||
-          Gitlab::CurrentSettings.current_application_settings.default_artifacts_expire_in
-
-        job.job_artifacts.build(
-          project: job.project,
-          file: artifacts,
-          file_type: params['artifact_type'],
-          file_format: params['artifact_format'],
-          file_sha256: artifacts.sha256,
-          expire_in: expire_in)
-
-        if metadata
-          job.job_artifacts.build(
-            project: job.project,
-            file: metadata,
-            file_type: :metadata,
-            file_format: :gzip,
-            file_sha256: metadata.sha256,
-            expire_in: expire_in)
-        end
-
-        if job.update(artifacts_expire_in: expire_in)
-          present Ci::BuildRunnerPresenter.new(job), with: Entities::JobRequest::Response
+        if Ci::CreateJobArtifactsService.new.execute(job, artifacts, params, metadata_file: metadata)
+          status :created
         else
           render_validation_error!(job)
         end

spec/requests/api/runner_spec.rb

@@ -1607,7 +1607,7 @@ describe API::Runner, :clean_gitlab_redis_shared_state do
                 it_behaves_like 'successful artifacts upload'
               end
 
-              context 'for file stored remotelly' do
+              context 'for file stored remotely' do
                 let!(:fog_connection) do
                   stub_artifacts_object_storage(direct_upload: true)
                 end
@@ -1894,6 +1894,46 @@ describe API::Runner, :clean_gitlab_redis_shared_state do
           end
         end
 
+        context 'when artifacts already exist for the job' do
+          let(:params) do
+            {
+              artifact_type: :archive,
+              artifact_format: :zip,
+              'file.sha256' => uploaded_sha256
+            }
+          end
+
+          let(:existing_sha256) { '0' * 64 }
+
+          let!(:existing_artifact) do
+            create(:ci_job_artifact, :archive, file_sha256: existing_sha256, job: job)
+          end
+
+          context 'when sha256 is the same of the existing artifact' do
+            let(:uploaded_sha256) { existing_sha256 }
+
+            it 'ignores the new artifact' do
+              upload_artifacts(file_upload, headers_with_token, params)
+
+              expect(response).to have_gitlab_http_status(:created)
+              expect(job.reload.job_artifacts_archive).to eq(existing_artifact)
+            end
+          end
+
+          context 'when sha256 is different than the existing artifact' do
+            let(:uploaded_sha256) { '1' * 64 }
+
+            it 'logs and returns an error' do
+              expect(Gitlab::ErrorTracking).to receive(:track_exception)
+
+              upload_artifacts(file_upload, headers_with_token, params)
+
+              expect(response).to have_gitlab_http_status(:bad_request)
+              expect(job.reload.job_artifacts_archive).to eq(existing_artifact)
+            end
+          end
+        end
+
         context 'when artifacts are being stored outside of tmp path' do
           let(:new_tmpdir) { Dir.mktmpdir }
 

spec/services/ci/create_job_artifacts_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Ci::CreateJobArtifactsService do
+  let(:service) { described_class.new }
+  let(:job) { create(:ci_build) }
+  let(:artifacts_sha256) { '0' * 64 }
+  let(:metadata_file) { nil }
+
+  let(:artifacts_file) do
+    file_to_upload('spec/fixtures/ci_build_artifacts.zip', sha256: artifacts_sha256)
+  end
+
+  let(:params) do
+    {
+      'artifact_type' => 'archive',
+      'artifact_format' => 'zip'
+    }
+  end
+
+  def file_to_upload(path, params = {})
+    upload = Tempfile.new('upload')
+    FileUtils.copy(path, upload.path)
+
+    UploadedFile.new(upload.path, params)
+  end
+
+  describe '#execute' do
+    subject { service.execute(job, artifacts_file, params, metadata_file: metadata_file) }
+
+    context 'when artifacts file is uploaded' do
+      it 'saves artifact for the given type' do
+        expect { subject }.to change { Ci::JobArtifact.count }.by(1)
+
+        new_artifact = job.job_artifacts.last
+        expect(new_artifact.project).to eq(job.project)
+        expect(new_artifact.file).to be_present
+        expect(new_artifact.file_type).to eq(params['artifact_type'])
+        expect(new_artifact.file_format).to eq(params['artifact_format'])
+        expect(new_artifact.file_sha256).to eq(artifacts_sha256)
+      end
+
+      context 'when metadata file is also uploaded' do
+        let(:metadata_file) do
+          file_to_upload('spec/fixtures/ci_build_artifacts_metadata.gz', sha256: artifacts_sha256)
+        end
+
+        before do
+          stub_application_setting(default_artifacts_expire_in: '1 day')
+        end
+
+        it 'saves metadata artifact' do
+          expect { subject }.to change { Ci::JobArtifact.count }.by(2)
+
+          new_artifact = job.job_artifacts.last
+          expect(new_artifact.project).to eq(job.project)
+          expect(new_artifact.file).to be_present
+          expect(new_artifact.file_type).to eq('metadata')
+          expect(new_artifact.file_format).to eq('gzip')
+          expect(new_artifact.file_sha256).to eq(artifacts_sha256)
+        end
+
+        it 'sets expiration date according to application settings' do
+          expected_expire_at = 1.day.from_now
+
+          expect(subject).to be_truthy
+          archive_artifact, metadata_artifact = job.job_artifacts.last(2)
+
+          expect(job.artifacts_expire_at).to be_within(1.minute).of(expected_expire_at)
+          expect(archive_artifact.expire_at).to be_within(1.minute).of(expected_expire_at)
+          expect(metadata_artifact.expire_at).to be_within(1.minute).of(expected_expire_at)
+        end
+
+        context 'when expire_in params is set' do
+          before do
+            params.merge!('expire_in' => '2 hours')
+          end
+
+          it 'sets expiration date according to the parameter' do
+            expected_expire_at = 2.hours.from_now
+
+            expect(subject).to be_truthy
+            archive_artifact, metadata_artifact = job.job_artifacts.last(2)
+
+            expect(job.artifacts_expire_at).to be_within(1.minute).of(expected_expire_at)
+            expect(archive_artifact.expire_at).to be_within(1.minute).of(expected_expire_at)
+            expect(metadata_artifact.expire_at).to be_within(1.minute).of(expected_expire_at)
+          end
+        end
+      end
+    end
+
+    context 'when artifacts file already exists' do
+      let!(:existing_artifact) do
+        create(:ci_job_artifact, :archive, file_sha256: existing_sha256, job: job)
+      end
+
+      context 'when sha256 of uploading artifact is the same of the existing one' do
+        let(:existing_sha256) { artifacts_sha256 }
+
+        it 'ignores the changes' do
+          expect { subject }.not_to change { Ci::JobArtifact.count }
+          expect(subject).to be_truthy
+        end
+      end
+
+      context 'when sha256 of uploading artifact is different than the existing one' do
+        let(:existing_sha256) { '1' * 64 }
+
+        it 'returns false and logs the error' do
+          expect(Gitlab::ErrorTracking).to receive(:track_exception).and_call_original
+
+          expect { subject }.not_to change { Ci::JobArtifact.count }
+          expect(subject).to be_falsey
+          expect(job.errors[:base]).to contain_exactly('another artifact of the same type already exists')
+        end
+      end
+    end
+  end
+end