Merge branch 'fix-direct-upload-for-old-records' into 'master'

Fix direct_upload for old records

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

7 files changed, 204 insertions, 47 deletions

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')