Fix direct_upload when records with null file_store are used

Old records have a null value of file_store column.
This causes the problems with current direct_upload implementation,
as this makes it to choose Store::REMOTE instead of Store::LOCAL.

This change moves the store save when change saving the object.

3 files changed, 23 insertions, 16 deletions

diff --git a/app/models/ci/job_artifact.rb b/app/models/ci/job_artifact.rb
index f846482cdeb..39676efa08c 100644
--- a/app/models/ci/job_artifact.rb
+++ b/app/models/ci/job_artifact.rb
@@ -7,14 +7,15 @@ 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?
     after_save :update_project_statistics_after_save, if: :size_changed?
     after_destroy :update_project_statistics_after_destroy, unless: :project_destroyed?
 
-    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
 
@@ -25,7 +26,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?