Move file store updates and mount_uploader into a concern

The pattern of updating the model's `file_store` column after `file` was
saved repeated across many different models. In one case, the
`after_save` was missing, which led to a bug preventing object store
from working with the dependency proxy
(https://gitlab.com/gitlab-org/gitlab/-/issues/231563). Consolidate the
logic and tests into a single place.

Closes https://gitlab.com/gitlab-org/gitlab/-/issues/232094

app/models/ci/job_artifact.rb

@@ -9,6 +9,7 @@ module Ci
     include Sortable
     include IgnorableColumns
     include Artifactable
+    include FileStoreMounter
     extend Gitlab::Ci::Model
 
     NotSupportedAdapterError = Class.new(StandardError)
@@ -115,7 +116,7 @@ module Ci
     belongs_to :project
     belongs_to :job, class_name: "Ci::Build", foreign_key: :job_id
 
-    mount_uploader :file, JobArtifactUploader
+    mount_file_store_uploader JobArtifactUploader
 
     validates :file_format, presence: true, unless: :trace?, on: :create
     validate :validate_supported_file_format!, on: :create
@@ -124,8 +125,6 @@ module Ci
 
     update_project_statistics project_statistics_name: :build_artifacts_size
 
-    after_save :update_file_store, if: :saved_change_to_file?
-
     scope :not_expired, -> { where('expire_at IS NULL OR expire_at > ?', Time.current) }
     scope :with_files_stored_locally, -> { where(file_store: ::JobArtifactUploader::Store::LOCAL) }
     scope :with_files_stored_remotely, -> { where(file_store: ::JobArtifactUploader::Store::REMOTE) }
@@ -229,12 +228,6 @@ module Ci
       end
     end
 
-    def update_file_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.associated_file_types_for(file_type)
       return unless file_types.include?(file_type)
 

app/models/concerns/file_store_mounter.rb

+# frozen_string_literal: true
+
+module FileStoreMounter
+  extend ActiveSupport::Concern
+
+  class_methods do
+    def mount_file_store_uploader(uploader)
+      mount_uploader(:file, uploader)
+
+      after_save :update_file_store, if: :saved_change_to_file?
+    end
+  end
+
+  private
+
+  def update_file_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
+end

app/models/lfs_object.rb

@@ -5,6 +5,7 @@ class LfsObject < ApplicationRecord
   include Checksummable
   include EachBatch
   include ObjectStorage::BackgroundMove
+  include FileStoreMounter
 
   has_many :lfs_objects_projects, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
   has_many :projects, -> { distinct }, through: :lfs_objects_projects
@@ -15,21 +16,13 @@ class LfsObject < ApplicationRecord
 
   validates :oid, presence: true, uniqueness: true
 
-  mount_uploader :file, LfsObjectUploader
-
-  after_save :update_file_store, if: :saved_change_to_file?
+  mount_file_store_uploader LfsObjectUploader
 
   def self.not_linked_to_project(project)
     where('NOT EXISTS (?)',
           project.lfs_objects_projects.select(1).where('lfs_objects_projects.lfs_object_id = lfs_objects.id'))
   end
 
-  def update_file_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)
     if project.fork_network_member
       lfs_objects_projects

app/models/terraform/state.rb

@@ -3,6 +3,7 @@
 module Terraform
   class State < ApplicationRecord
     include UsageStatistics
+    include FileStoreMounter
 
     DEFAULT = '{"version":1}'.freeze
     HEX_REGEXP = %r{\A\h+\z}.freeze
@@ -17,18 +18,10 @@ module Terraform
 
     default_value_for(:uuid, allows_nil: false) { SecureRandom.hex(UUID_LENGTH / 2) }
 
-    after_save :update_file_store, if: :saved_change_to_file?
-
-    mount_uploader :file, StateUploader
+    mount_file_store_uploader StateUploader
 
     default_value_for(:file) { CarrierWaveStringFile.new(DEFAULT) }
 
-    def update_file_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 file_store
       super || StateUploader.default_store
     end

changelogs/unreleased/sh-refactor-file-store-mounter.yml

+---
+title: Move file store updates and mount_uploader into a concern
+merge_request: 37907
+author:
+type: other

ee/app/models/dependency_proxy/blob.rb

 # frozen_string_literal: true
 
 class DependencyProxy::Blob < ApplicationRecord
+  include FileStoreMounter
+
   belongs_to :group
 
   validates :group, presence: true
   validates :file, presence: true
   validates :file_name, presence: true
 
-  mount_uploader :file, DependencyProxy::FileUploader
-
-  after_save :update_file_store, if: :saved_change_to_file?
+  mount_file_store_uploader DependencyProxy::FileUploader
 
   def self.total_size
     sum(:size)
@@ -18,10 +18,4 @@ class DependencyProxy::Blob < ApplicationRecord
   def self.find_or_build(file_name)
     find_or_initialize_by(file_name: file_name)
   end
-
-  def update_file_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
 end

ee/app/models/vulnerabilities/export.rb

@@ -2,15 +2,15 @@
 
 module Vulnerabilities
   class Export < ApplicationRecord
+    include FileStoreMounter
+
     self.table_name = "vulnerability_exports"
 
     belongs_to :project
     belongs_to :group
     belongs_to :author, optional: false, class_name: 'User'
 
-    mount_uploader :file, AttachmentUploader
-
-    after_save :update_file_store, if: :saved_change_to_file?
+    mount_file_store_uploader AttachmentUploader
 
     enum format: {
       csv: 0
@@ -77,12 +77,6 @@ module Vulnerabilities
       Upload.find_by(model: self, path: paths)
     end
 
-    def update_file_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
-
     private
 
     def make_project_level_export(project)

ee/spec/models/dependency_proxy/blob_spec.rb

@@ -41,11 +41,7 @@ RSpec.describe DependencyProxy::Blob, type: :model do
     subject { create(:dependency_proxy_blob) }
 
     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
+      it_behaves_like 'mounted file in local store'
     end
 
     context 'when direct upload is enabled' do
@@ -53,13 +49,7 @@ RSpec.describe DependencyProxy::Blob, type: :model do
         stub_dependency_proxy_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
+      it_behaves_like 'mounted file in object store'
     end
   end
 end

spec/models/ci/job_artifact_spec.rb

@@ -483,11 +483,7 @@ RSpec.describe Ci::JobArtifact do
     subject { create(:ci_job_artifact, :archive) }
 
     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
+      it_behaves_like 'mounted file in local store'
     end
 
     context 'when direct upload is enabled' do
@@ -496,11 +492,7 @@ RSpec.describe Ci::JobArtifact do
       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
+        it_behaves_like 'mounted file in object store'
       end
     end
   end

spec/models/lfs_object_spec.rb

@@ -152,14 +152,10 @@ RSpec.describe LfsObject do
     end
 
     describe 'file is being stored' do
-      let(:lfs_object) { create(:lfs_object, :with_file) }
+      subject { create(:lfs_object, :with_file) }
 
       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
+        it_behaves_like 'mounted file in local store'
       end
 
       context 'when direct upload is enabled' do
@@ -167,13 +163,7 @@ RSpec.describe LfsObject 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
+        it_behaves_like 'mounted file in object store'
       end
     end
   end

spec/models/terraform/state_spec.rb

@@ -45,9 +45,7 @@ RSpec.describe Terraform::State do
 
   describe '#update_file_store' do
     context 'when file is stored in object storage' do
-      it 'sets file_store to remote' do
-        expect(subject.file_store).to eq(ObjectStorage::Store::REMOTE)
-      end
+      it_behaves_like 'mounted file in object store'
     end
 
     context 'when file is stored locally' do
@@ -55,9 +53,7 @@ RSpec.describe Terraform::State do
         stub_terraform_state_object_storage(Terraform::StateUploader, enabled: false)
       end
 
-      it 'sets file_store to local' do
-        expect(subject.file_store).to eq(ObjectStorage::Store::LOCAL)
-      end
+      it_behaves_like 'mounted file in local store'
     end
   end
 end

spec/support/shared_examples/models/concerns/file_store_mounter_shared_examples.rb

+# frozen_string_literal: true
+
+RSpec.shared_examples 'mounted file in 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
+
+RSpec.shared_examples 'mounted file in object store' 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