Merge branch '46147-remove-model-redefinition-worker' into 'master'

Resolve "NoMethodError: undefined method `uploader_context' for #<ObjectStorage::MigrateUploadsWorker::Upload:0x00007f59e..."

Closes #46147

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

2 files changed, 95 insertions, 149 deletions

diff --git a/app/workers/object_storage/migrate_uploads_worker.rb b/app/workers/object_storage/migrate_uploads_worker.rb
index a6b2c251254..a3ecfa8e711 100644
--- a/app/workers/object_storage/migrate_uploads_worker.rb
+++ b/app/workers/object_storage/migrate_uploads_worker.rb
@@ -9,85 +9,6 @@ module ObjectStorage
 
     SanityCheckError = Class.new(StandardError)
 
-    class Upload < ActiveRecord::Base
-      # Upper limit for foreground checksum processing
-      CHECKSUM_THRESHOLD = 100.megabytes
-
-      belongs_to :model, polymorphic: true # rubocop:disable Cop/PolymorphicAssociations
-
-      validates :size, presence: true
-      validates :path, presence: true
-      validates :model, presence: true
-      validates :uploader, presence: true
-
-      before_save  :calculate_checksum!, if: :foreground_checksummable?
-      after_commit :schedule_checksum,   if: :checksummable?
-
-      scope :stored_locally, -> { where(store: [nil, ObjectStorage::Store::LOCAL]) }
-      scope :stored_remotely, -> { where(store: ObjectStorage::Store::REMOTE) }
-
-      def self.hexdigest(path)
-        Digest::SHA256.file(path).hexdigest
-      end
-
-      def absolute_path
-        raise ObjectStorage::RemoteStoreError, "Remote object has no absolute path." unless local?
-        return path unless relative_path?
-
-        uploader_class.absolute_path(self)
-      end
-
-      def calculate_checksum!
-        self.checksum = nil
-        return unless checksummable?
-
-        self.checksum = self.class.hexdigest(absolute_path)
-      end
-
-      def build_uploader(mounted_as = nil)
-        uploader_class.new(model, mounted_as).tap do |uploader|
-          uploader.upload = self
-          uploader.retrieve_from_store!(identifier)
-        end
-      end
-
-      def exist?
-        File.exist?(absolute_path)
-      end
-
-      def local?
-        return true if store.nil?
-
-        store == ObjectStorage::Store::LOCAL
-      end
-
-      private
-
-      def checksummable?
-        checksum.nil? && local? && exist?
-      end
-
-      def foreground_checksummable?
-        checksummable? && size <= CHECKSUM_THRESHOLD
-      end
-
-      def schedule_checksum
-        UploadChecksumWorker.perform_async(id)
-      end
-
-      def relative_path?
-        !path.start_with?('/')
-      end
-
-      def identifier
-        File.basename(path)
-      end
-
-      def uploader_class
-        Object.const_get(uploader)
-      end
-    end
-
     class MigrationResult
       attr_reader :upload
       attr_accessor :error
diff --git a/spec/uploaders/workers/object_storage/migrate_uploads_worker_spec.rb b/spec/uploaders/workers/object_storage/migrate_uploads_worker_spec.rb
index 7a7dcb71680..aed62f97448 100644
--- a/spec/uploaders/workers/object_storage/migrate_uploads_worker_spec.rb
+++ b/spec/uploaders/workers/object_storage/migrate_uploads_worker_spec.rb
@@ -7,113 +7,138 @@ describe ObjectStorage::MigrateUploadsWorker, :sidekiq do
     end
   end
 
-  let!(:projects) { create_list(:project, 10, :with_avatar) }
-  let(:uploads) { Upload.all }
   let(:model_class) { Project }
-  let(:mounted_as) { :avatar }
+  let(:uploads) { Upload.all }
   let(:to_store) { ObjectStorage::Store::REMOTE }
 
-  before do
-    stub_uploads_object_storage(AvatarUploader)
-  end
-
-  describe '.enqueue!' do
-    def enqueue!
-      described_class.enqueue!(uploads, Project, mounted_as, to_store)
-    end
+  shared_examples "uploads migration worker" do
+    describe '.enqueue!' do
+      def enqueue!
+        described_class.enqueue!(uploads, Project, mounted_as, to_store)
+      end
 
-    it 'is guarded by .sanity_check!' do
-      expect(described_class).to receive(:perform_async)
-      expect(described_class).to receive(:sanity_check!)
+      it 'is guarded by .sanity_check!' do
+        expect(described_class).to receive(:perform_async)
+        expect(described_class).to receive(:sanity_check!)
 
-      enqueue!
-    end
+        enqueue!
+      end
 
-    context 'sanity_check! fails' do
-      include_context 'sanity_check! fails'
+      context 'sanity_check! fails' do
+        include_context 'sanity_check! fails'
 
-      it 'does not enqueue a job' do
-        expect(described_class).not_to receive(:perform_async)
+        it 'does not enqueue a job' do
+          expect(described_class).not_to receive(:perform_async)
 
-        expect { enqueue! }.to raise_error(described_class::SanityCheckError)
+          expect { enqueue! }.to raise_error(described_class::SanityCheckError)
+        end
       end
     end
-  end
 
-  describe '.sanity_check!' do
-    shared_examples 'raises a SanityCheckError' do
-      let(:mount_point) { nil }
+    describe '.sanity_check!' do
+      shared_examples 'raises a SanityCheckError' do
+        let(:mount_point) { nil }
 
-      it do
-        expect { described_class.sanity_check!(uploads, model_class, mount_point) }
-          .to raise_error(described_class::SanityCheckError)
+        it do
+          expect { described_class.sanity_check!(uploads, model_class, mount_point) }
+            .to raise_error(described_class::SanityCheckError)
+        end
       end
-    end
 
-    context 'uploader types mismatch' do
-      let!(:outlier) { create(:upload, uploader: 'FileUploader') }
+      before do
+        stub_const("WrongModel", Class.new)
+      end
 
-      include_examples 'raises a SanityCheckError'
-    end
+      context 'uploader types mismatch' do
+        let!(:outlier) { create(:upload, uploader: 'GitlabUploader') }
 
-    context 'model types mismatch' do
-      let!(:outlier) { create(:upload, model_type: 'Potato') }
+        include_examples 'raises a SanityCheckError'
+      end
 
-      include_examples 'raises a SanityCheckError'
-    end
+      context 'model types mismatch' do
+        let!(:outlier) { create(:upload, model_type: 'WrongModel') }
 
-    context 'mount point not found' do
-      include_examples 'raises a SanityCheckError' do
-        let(:mount_point) { :potato }
+        include_examples 'raises a SanityCheckError'
       end
-    end
-  end
 
-  describe '#perform' do
-    def perform
-      described_class.new.perform(uploads.ids, model_class.to_s, mounted_as, to_store)
-    rescue ObjectStorage::MigrateUploadsWorker::Report::MigrationFailures
-      # swallow
+      context 'mount point not found' do
+        include_examples 'raises a SanityCheckError' do
+          let(:mount_point) { :potato }
+        end
+      end
     end
 
-    shared_examples 'outputs correctly' do |success: 0, failures: 0|
-      total = success + failures
+    describe '#perform' do
+      def perform
+        described_class.new.perform(uploads.ids, model_class.to_s, mounted_as, to_store)
+      rescue ObjectStorage::MigrateUploadsWorker::Report::MigrationFailures
+        # swallow
+      end
+
+      shared_examples 'outputs correctly' do |success: 0, failures: 0|
+        total = success + failures
 
-      if success > 0
-        it 'outputs the reports' do
-          expect(Rails.logger).to receive(:info).with(%r{Migrated #{success}/#{total} files})
+        if success > 0
+          it 'outputs the reports' do
+            expect(Rails.logger).to receive(:info).with(%r{Migrated #{success}/#{total} files})
 
-          perform
+            perform
+          end
         end
-      end
 
-      if failures > 0
-        it 'outputs upload failures' do
-          expect(Rails.logger).to receive(:warn).with(/Error .* I am a teapot/)
+        if failures > 0
+          it 'outputs upload failures' do
+            expect(Rails.logger).to receive(:warn).with(/Error .* I am a teapot/)
 
-          perform
+            perform
+          end
         end
       end
-    end
 
-    it_behaves_like 'outputs correctly', success: 10
+      it_behaves_like 'outputs correctly', success: 10
+
+      it 'migrates files' do
+        perform
 
-    it 'migrates files' do
-      perform
+        expect(Upload.where(store: ObjectStorage::Store::LOCAL).count).to eq(0)
+      end
 
-      aggregate_failures do
-        projects.each do |project|
-          expect(project.reload.avatar.upload.local?).to be_falsey
+      context 'migration is unsuccessful' do
+        before do
+          allow_any_instance_of(ObjectStorage::Concern)
+            .to receive(:migrate!).and_raise(CarrierWave::UploadError, "I am a teapot.")
         end
+
+        it_behaves_like 'outputs correctly', failures: 10
       end
     end
+  end
 
-    context 'migration is unsuccessful' do
-      before do
-        allow_any_instance_of(ObjectStorage::Concern).to receive(:migrate!).and_raise(CarrierWave::UploadError, "I am a teapot.")
-      end
+  context "for AvatarUploader" do
+    let!(:projects) { create_list(:project, 10, :with_avatar) }
+    let(:mounted_as) { :avatar }
 
-      it_behaves_like 'outputs correctly', failures: 10
+    before do
+      stub_uploads_object_storage(AvatarUploader)
+    end
+
+    it_behaves_like "uploads migration worker"
+  end
+
+  context "for FileUploader" do
+    let!(:projects) { create_list(:project, 10) }
+    let(:secret) { SecureRandom.hex }
+    let(:mounted_as) { nil }
+
+    before do
+      stub_uploads_object_storage(FileUploader)
+
+      projects.map do |project|
+        uploader = FileUploader.new(project)
+        uploader.store!(fixture_file_upload('spec/fixtures/doc_sample.txt'))
+      end
     end
+
+    it_behaves_like "uploads migration worker"
   end
 end