From c30de8d6ed74c969a09227172aee8bfb2cc64c88 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 19 Apr 2018 10:58:04 +0000 Subject: Merge branch 'fix-direct-upload-for-old-records' into 'master' Fix direct_upload for old records See merge request gitlab-org/gitlab-ce!18360 --- app/models/ci/job_artifact.rb | 11 ++- app/models/lfs_object.rb | 6 +- app/uploaders/object_storage.rb | 22 +++-- .../fix-direct-upload-for-old-records.yml | 5 + spec/models/ci/job_artifact_spec.rb | 67 ++++++++++++++ spec/models/lfs_object_spec.rb | 39 ++++++++ spec/uploaders/object_storage_spec.rb | 101 ++++++++++++++------- 7 files changed, 204 insertions(+), 47 deletions(-) create mode 100644 changelogs/unreleased/fix-direct-upload-for-old-records.yml diff --git a/app/models/ci/job_artifact.rb b/app/models/ci/job_artifact.rb index fbb95fe16df..151a137debb 100644 --- a/app/models/ci/job_artifact.rb +++ b/app/models/ci/job_artifact.rb @@ -7,12 +7,13 @@ module Ci belongs_to :project belongs_to :job, class_name: "Ci::Build", foreign_key: :job_id - before_save :update_file_store + mount_uploader :file, JobArtifactUploader + before_save :set_size, if: :file_changed? - scope :with_files_stored_locally, -> { where(file_store: [nil, ::JobArtifactUploader::Store::LOCAL]) } + after_save :update_file_store - mount_uploader :file, JobArtifactUploader + scope :with_files_stored_locally, -> { where(file_store: [nil, ::JobArtifactUploader::Store::LOCAL]) } delegate :exists?, :open, to: :file @@ -23,7 +24,9 @@ module Ci } def update_file_store - self.file_store = file.object_store + # The file.object_store is set during `uploader.store!` + # which happens after object is inserted/updated + self.update_column(:file_store, file.object_store) end def self.artifacts_size_for(project) diff --git a/app/models/lfs_object.rb b/app/models/lfs_object.rb index b7de46fa202..6b7f280fb70 100644 --- a/app/models/lfs_object.rb +++ b/app/models/lfs_object.rb @@ -11,10 +11,12 @@ class LfsObject < ActiveRecord::Base mount_uploader :file, LfsObjectUploader - before_save :update_file_store + after_save :update_file_store def update_file_store - self.file_store = file.object_store + # The file.object_store is set during `uploader.store!` + # which happens after object is inserted/updated + self.update_column(:file_store, file.object_store) end def project_allowed_access?(project) diff --git a/app/uploaders/object_storage.rb b/app/uploaders/object_storage.rb index bd258e04d3f..a3549cada95 100644 --- a/app/uploaders/object_storage.rb +++ b/app/uploaders/object_storage.rb @@ -183,14 +183,6 @@ module ObjectStorage StoreURL: connection.put_object_url(remote_store_path, upload_path, expire_at, options) } end - - def default_object_store - if self.object_store_enabled? && self.direct_upload_enabled? - Store::REMOTE - else - Store::LOCAL - end - end end # allow to configure and overwrite the filename @@ -211,12 +203,13 @@ module ObjectStorage end def object_store - @object_store ||= model.try(store_serialization_column) || self.class.default_object_store + # We use Store::LOCAL as null value indicates the local storage + @object_store ||= model.try(store_serialization_column) || Store::LOCAL end # rubocop:disable Gitlab/ModuleWithInstanceVariables def object_store=(value) - @object_store = value || self.class.default_object_store + @object_store = value || Store::LOCAL @storage = storage_for(object_store) end # rubocop:enable Gitlab/ModuleWithInstanceVariables @@ -302,6 +295,15 @@ module ObjectStorage super end + def store!(new_file = nil) + # when direct upload is enabled, always store on remote storage + if self.class.object_store_enabled? && self.class.direct_upload_enabled? + self.object_store = Store::REMOTE + end + + super + end + private def schedule_background_upload? diff --git a/changelogs/unreleased/fix-direct-upload-for-old-records.yml b/changelogs/unreleased/fix-direct-upload-for-old-records.yml new file mode 100644 index 00000000000..a062b9e73e9 --- /dev/null +++ b/changelogs/unreleased/fix-direct-upload-for-old-records.yml @@ -0,0 +1,5 @@ +--- +title: Fix direct_upload when records with null file_store are used +merge_request: +author: +type: fixed diff --git a/spec/models/ci/job_artifact_spec.rb b/spec/models/ci/job_artifact_spec.rb index 1aa28434879..817b74ad07a 100644 --- a/spec/models/ci/job_artifact_spec.rb +++ b/spec/models/ci/job_artifact_spec.rb @@ -118,4 +118,71 @@ describe Ci::JobArtifact do is_expected.to be_nil end end + + context 'when destroying the artifact' do + let(:project) { create(:project, :repository) } + let(:pipeline) { create(:ci_pipeline, project: project) } + let!(:build) { create(:ci_build, :artifacts, pipeline: pipeline) } + + it 'updates the project statistics' do + artifact = build.job_artifacts.first + + expect(ProjectStatistics) + .to receive(:increment_statistic) + .and_call_original + + expect { artifact.destroy } + .to change { project.statistics.reload.build_artifacts_size } + .by(-106365) + end + + context 'when it is destroyed from the project level' do + it 'does not update the project statistics' do + expect(ProjectStatistics) + .not_to receive(:increment_statistic) + + project.update_attributes(pending_delete: true) + project.destroy! + end + end + end + + describe 'file is being stored' do + subject { create(:ci_job_artifact, :archive) } + + context 'when object has nil store' do + before do + subject.update_column(:file_store, nil) + subject.reload + end + + it 'is stored locally' do + expect(subject.file_store).to be(nil) + expect(subject.file).to be_file_storage + expect(subject.file.object_store).to eq(ObjectStorage::Store::LOCAL) + end + end + + context 'when existing object has local store' do + it 'is stored locally' do + expect(subject.file_store).to be(ObjectStorage::Store::LOCAL) + expect(subject.file).to be_file_storage + expect(subject.file.object_store).to eq(ObjectStorage::Store::LOCAL) + end + end + + context 'when direct upload is enabled' do + before do + stub_artifacts_object_storage(direct_upload: true) + end + + context 'when file is stored' do + it 'is stored remotely' do + expect(subject.file_store).to eq(ObjectStorage::Store::REMOTE) + expect(subject.file).not_to be_file_storage + expect(subject.file.object_store).to eq(ObjectStorage::Store::REMOTE) + end + end + end + end end diff --git a/spec/models/lfs_object_spec.rb b/spec/models/lfs_object_spec.rb index a182116d637..ba06ff42d87 100644 --- a/spec/models/lfs_object_spec.rb +++ b/spec/models/lfs_object_spec.rb @@ -81,5 +81,44 @@ describe LfsObject do end end end + + describe 'file is being stored' do + let(:lfs_object) { create(:lfs_object, :with_file) } + + context 'when object has nil store' do + before do + lfs_object.update_column(:file_store, nil) + lfs_object.reload + end + + it 'is stored locally' do + expect(lfs_object.file_store).to be(nil) + expect(lfs_object.file).to be_file_storage + expect(lfs_object.file.object_store).to eq(ObjectStorage::Store::LOCAL) + end + end + + context 'when existing object has local store' do + it 'is stored locally' do + expect(lfs_object.file_store).to be(ObjectStorage::Store::LOCAL) + expect(lfs_object.file).to be_file_storage + expect(lfs_object.file.object_store).to eq(ObjectStorage::Store::LOCAL) + end + end + + context 'when direct upload is enabled' do + before do + stub_lfs_object_storage(direct_upload: true) + end + + context 'when file is stored' do + it 'is stored remotely' do + expect(lfs_object.file_store).to eq(ObjectStorage::Store::REMOTE) + expect(lfs_object.file).not_to be_file_storage + expect(lfs_object.file.object_store).to eq(ObjectStorage::Store::REMOTE) + end + end + end + end end end diff --git a/spec/uploaders/object_storage_spec.rb b/spec/uploaders/object_storage_spec.rb index 16455e2517b..e7277b337f6 100644 --- a/spec/uploaders/object_storage_spec.rb +++ b/spec/uploaders/object_storage_spec.rb @@ -75,36 +75,8 @@ describe ObjectStorage do expect(object).to receive(:file_store).and_return(nil) end - context 'when object storage is enabled' do - context 'when direct uploads are enabled' do - before do - stub_uploads_object_storage(uploader_class, enabled: true, direct_upload: true) - end - - it "uses Store::REMOTE" do - is_expected.to eq(described_class::Store::REMOTE) - end - end - - context 'when direct uploads are disabled' do - before do - stub_uploads_object_storage(uploader_class, enabled: true, direct_upload: false) - end - - it "uses Store::LOCAL" do - is_expected.to eq(described_class::Store::LOCAL) - end - end - end - - context 'when object storage is disabled' do - before do - stub_uploads_object_storage(uploader_class, enabled: false) - end - - it "uses Store::LOCAL" do - is_expected.to eq(described_class::Store::LOCAL) - end + it "uses Store::LOCAL" do + is_expected.to eq(described_class::Store::LOCAL) end end @@ -537,6 +509,72 @@ describe ObjectStorage do end end + context 'when local file is used' do + let(:temp_file) { Tempfile.new("test") } + + before do + FileUtils.touch(temp_file) + end + + after do + FileUtils.rm_f(temp_file) + end + + context 'when valid file is used' do + context 'when valid file is specified' do + let(:uploaded_file) { temp_file } + + context 'when object storage and direct upload is specified' do + before do + stub_uploads_object_storage(uploader_class, enabled: true, direct_upload: true) + end + + context 'when file is stored' do + subject do + uploader.store!(uploaded_file) + end + + it 'file to be remotely stored in permament location' do + subject + + expect(uploader).to be_exists + expect(uploader).not_to be_cached + expect(uploader).not_to be_file_storage + expect(uploader.path).not_to be_nil + expect(uploader.path).not_to include('tmp/upload') + expect(uploader.path).not_to include('tmp/cache') + expect(uploader.object_store).to eq(described_class::Store::REMOTE) + end + end + end + + context 'when object storage and direct upload is not used' do + before do + stub_uploads_object_storage(uploader_class, enabled: true, direct_upload: false) + end + + context 'when file is stored' do + subject do + uploader.store!(uploaded_file) + end + + it 'file to be remotely stored in permament location' do + subject + + expect(uploader).to be_exists + expect(uploader).not_to be_cached + expect(uploader).to be_file_storage + expect(uploader.path).not_to be_nil + expect(uploader.path).not_to include('tmp/upload') + expect(uploader.path).not_to include('tmp/cache') + expect(uploader.object_store).to eq(described_class::Store::LOCAL) + end + end + end + end + end + end + context 'when remote file is used' do let(:temp_file) { Tempfile.new("test") } @@ -590,9 +628,9 @@ describe ObjectStorage do expect(uploader).to be_exists expect(uploader).to be_cached + expect(uploader).not_to be_file_storage expect(uploader.path).not_to be_nil expect(uploader.path).not_to include('tmp/cache') - expect(uploader.url).not_to be_nil expect(uploader.path).not_to include('tmp/cache') expect(uploader.object_store).to eq(described_class::Store::REMOTE) end @@ -607,6 +645,7 @@ describe ObjectStorage do expect(uploader).to be_exists expect(uploader).not_to be_cached + expect(uploader).not_to be_file_storage expect(uploader.path).not_to be_nil expect(uploader.path).not_to include('tmp/upload') expect(uploader.path).not_to include('tmp/cache') -- cgit v1.2.1