Refactor FileRegistryRemoval service and its specs

It refactors FileRegistryRemoval but mostly we optimized
its spec

ee/app/services/geo/file_registry_removal_service.rb

 # frozen_string_literal: true
 
 module Geo
+  ##
+  ## Geo::FileRegistryRemovalService handles blob removal from a secondary node,
+  ## the file itself and the database records.
+  ## It handles all the possible combinations of 4 variables:
+  ## * Whether Model record exists
+  ## * Whether Registry Record exists
+  ## * Whether the file on a storage exists
+  ## * Whether the file_path is passed (RegistryConsistencyWorker doesn't pass one)
+  ## In all the cases the best effort should have to be made.
+  ##
   class FileRegistryRemovalService < BaseFileService
     include ::Gitlab::Utils::StrongMemoize
 
@@ -73,16 +83,10 @@ module Geo
       end
     end
 
-    def blob_path_from_replicator
-      replicator.blob_path
-    rescue ActiveRecord::RecordNotFound
-      nil
-    end
-
     def file_path
       strong_memoize(:file_path) do
         next @object_file_path if @object_file_path
-        next blob_path_from_replicator if replicator
+
         # When local storage is used, just rely on the existing methods
         next if file_uploader.nil?
         next file_uploader.file.path if file_uploader.object_store == ObjectStorage::Store::LOCAL
@@ -93,6 +97,8 @@ module Geo
 
     def file_uploader
       strong_memoize(:file_uploader) do
+        next replicator.carrierwave_uploader if replicator
+
         case object_type
         when :job_artifact
           Ci::JobArtifact.find(object_db_id).file

ee/spec/services/geo/file_registry_removal_service_spec.rb

@@ -21,115 +21,43 @@ RSpec.describe Geo::FileRegistryRemovalService, :geo do
       described_class.new(:lfs, 99).execute
     end
 
-    shared_examples 'removes artifact' do
-      subject(:service) { described_class.new('job_artifact', registry.artifact_id) }
-
-      before do
-        stub_exclusive_lease("file_registry_removal_service:job_artifact:#{registry.artifact_id}",
-          timeout: Geo::FileRegistryRemovalService::LEASE_TIMEOUT)
-      end
-
-      it 'file from disk' do
-        expect do
-          service.execute
-        end.to change { File.exist?(file_path) }.from(true).to(false)
-      end
-
-      it 'deletes registry entry' do
-        expect do
-          service.execute
-        end.to change(Geo::JobArtifactRegistry, :count).by(-1)
-      end
-    end
-
-    shared_examples 'removes artifact registry' do
-      subject(:service) { described_class.new('job_artifact', registry.artifact_id) }
-
-      before do
-        stub_exclusive_lease("file_registry_removal_service:job_artifact:#{registry.artifact_id}",
-          timeout: Geo::FileRegistryRemovalService::LEASE_TIMEOUT)
-      end
-
-      it 'deletes registry entry' do
-        expect do
-          service.execute
-        end.to change(Geo::JobArtifactRegistry, :count).by(-1)
-      end
-    end
-
-    shared_examples 'removes LFS object' do
-      subject(:service) { described_class.new('lfs', registry.lfs_object_id) }
-
-      before do
-        stub_exclusive_lease("file_registry_removal_service:lfs:#{registry.lfs_object_id}",
-          timeout: Geo::FileRegistryRemovalService::LEASE_TIMEOUT)
-      end
-
-      it 'file from disk' do
-        expect do
-          service.execute
-        end.to change { File.exist?(file_path) }.from(true).to(false)
-      end
-
-      it 'deletes registry entry' do
-        expect do
-          service.execute
-        end.to change(Geo::LfsObjectRegistry, :count).by(-1)
-      end
-    end
-
-    shared_examples 'removes LFS object registry' do
-      subject(:service) { described_class.new('lfs', registry.lfs_object_id) }
-
-      before do
-        stub_exclusive_lease("file_registry_removal_service:lfs:#{registry.lfs_object_id}",
-          timeout: Geo::FileRegistryRemovalService::LEASE_TIMEOUT)
-      end
-
-      it 'deletes registry entry' do
-        expect do
-          service.execute
-        end.to change(Geo::LfsObjectRegistry, :count).by(-1)
-      end
-    end
-
-    shared_examples 'removes package file' do
-      subject(:service) { described_class.new('package_file', registry.package_file_id) }
+    context 'with job artifact' do
+      shared_examples 'removes artifact' do
+        subject(:service) { described_class.new('job_artifact', registry.artifact_id) }
 
-      before do
-        stub_exclusive_lease("file_registry_removal_service:package_file:#{registry.package_file_id}",
-          timeout: Geo::FileRegistryRemovalService::LEASE_TIMEOUT)
-      end
+        before do
+          stub_exclusive_lease("file_registry_removal_service:job_artifact:#{registry.artifact_id}",
+            timeout: Geo::FileRegistryRemovalService::LEASE_TIMEOUT)
+        end
 
-      it 'file from disk' do
-        expect do
-          service.execute
-        end.to change { File.exist?(file_path) }.from(true).to(false)
-      end
+        it 'file from disk' do
+          expect do
+            service.execute
+          end.to change { File.exist?(file_path) }.from(true).to(false)
+        end
 
-      it 'deletes registry entry' do
-        expect do
-          service.execute
-        end.to change(Geo::PackageFileRegistry, :count).by(-1)
+        it 'deletes registry entry' do
+          expect do
+            service.execute
+          end.to change(Geo::JobArtifactRegistry, :count).by(-1)
+        end
       end
-    end
 
-    shared_examples 'removes package file registry' do
-      subject(:service) { described_class.new('package_file', registry.package_file_id) }
+      shared_examples 'removes artifact registry' do
+        subject(:service) { described_class.new('job_artifact', registry.artifact_id) }
 
-      before do
-        stub_exclusive_lease("file_registry_removal_service:package_file:#{registry.package_file_id}",
-          timeout: Geo::FileRegistryRemovalService::LEASE_TIMEOUT)
-      end
+        before do
+          stub_exclusive_lease("file_registry_removal_service:job_artifact:#{registry.artifact_id}",
+            timeout: Geo::FileRegistryRemovalService::LEASE_TIMEOUT)
+        end
 
-      it 'deletes registry entry' do
-        expect do
-          service.execute
-        end.to change(Geo::PackageFileRegistry, :count).by(-1)
+        it 'deletes registry entry' do
+          expect do
+            service.execute
+          end.to change(Geo::JobArtifactRegistry, :count).by(-1)
+        end
       end
-    end
 
-    context 'with job artifact' do
       let!(:job_artifact) { create(:ci_job_artifact, :archive) }
       let!(:registry) { create(:geo_job_artifact_registry, artifact_id: job_artifact.id) }
       let!(:file_path) { job_artifact.file.path }
@@ -138,20 +66,14 @@ RSpec.describe Geo::FileRegistryRemovalService, :geo do
 
       context 'migrated to object storage' do
         before do
-          stub_artifacts_object_storage
-          job_artifact.update_column(:file_store, JobArtifactUploader::Store::REMOTE)
-        end
-
-        it_behaves_like 'removes artifact'
-      end
-
-      context 'migrated to object storage' do
-        before do
-          stub_artifacts_object_storage
-          job_artifact.update_column(:file_store, LfsObjectUploader::Store::REMOTE)
+          job_artifact.update_column(:file_store, ObjectStorage::Store::REMOTE)
         end
 
         context 'with object storage enabled' do
+          before do
+            stub_artifacts_object_storage
+          end
+
           it_behaves_like 'removes artifact'
         end
 
@@ -172,30 +94,52 @@ RSpec.describe Geo::FileRegistryRemovalService, :geo do
         it_behaves_like 'removes artifact' do
           subject(:service) { described_class.new('job_artifact', registry.artifact_id, file_path) }
         end
+
+        it_behaves_like 'removes artifact registry'
       end
+    end
+
+    context 'with package file' do
+      shared_examples 'removes package file' do
+        subject(:service) { described_class.new('package_file', registry.package_file_id) }
 
-      context 'with orphaned registry' do
         before do
-          job_artifact.delete
+          stub_exclusive_lease("file_registry_removal_service:package_file:#{registry.package_file_id}",
+            timeout: Geo::FileRegistryRemovalService::LEASE_TIMEOUT)
         end
 
-        it_behaves_like 'removes artifact registry' do
-          subject(:service) { described_class.new('job_artifact', registry.artifact_id) }
+        it 'file from disk' do
+          expect do
+            service.execute
+          end.to change { File.exist?(file_path) }.from(true).to(false)
+        end
+
+        it 'deletes registry entry' do
+          expect do
+            service.execute
+          end.to change(Geo::PackageFileRegistry, :count).by(-1)
         end
       end
-    end
 
-    context 'with package file' do
-      let(:package_file) { create(:package_file_with_file) }
-      let!(:registry) { create(:geo_package_file_registry, package_file: package_file) }
-      let(:file_path) { Tempfile.new.path }
+      shared_examples 'removes package file registry' do
+        subject(:service) { described_class.new('package_file', registry.package_file_id) }
+
+        before do
+          stub_exclusive_lease("file_registry_removal_service:package_file:#{registry.package_file_id}",
+            timeout: Geo::FileRegistryRemovalService::LEASE_TIMEOUT)
+        end
 
-      before do
-        allow_next_instance_of(Geo::PackageFileReplicator) do |replicator|
-          allow(replicator).to receive(:blob_path).and_return(file_path)
+        it 'deletes registry entry' do
+          expect do
+            service.execute
+          end.to change(Geo::PackageFileRegistry, :count).by(-1)
         end
       end
 
+      let(:package_file) { create(:package_file_with_file) }
+      let!(:registry) { create(:geo_package_file_registry, package_file: package_file) }
+      let(:file_path) { package_file.file.path }
+
       it_behaves_like 'removes package file'
 
       context 'no package file record' do
@@ -220,27 +164,80 @@ RSpec.describe Geo::FileRegistryRemovalService, :geo do
     end
 
     context 'with uploads' do
-      let!(:upload) { create(:user, :with_avatar).avatar.upload }
+      shared_examples 'removes upload' do
+        subject(:service) { described_class.new('upload', registry.file_id, file_path) }
+
+        before do
+          stub_exclusive_lease("file_registry_removal_service:upload:#{registry.file_id}",
+            timeout: Geo::FileRegistryRemovalService::LEASE_TIMEOUT)
+        end
+
+        it 'file from disk' do
+          expect do
+            service.execute
+          end.to change { File.exist?(file_path) }.from(true).to(false)
+        end
+
+        it 'deletes registry entry' do
+          expect do
+            service.execute
+          end.to change(Geo::UploadRegistry, :count).by(-1)
+        end
+      end
+
+      shared_examples 'removes upload registry' do
+        subject(:service) { described_class.new('upload', registry.file_id, file_path) }
+
+        before do
+          stub_exclusive_lease("file_registry_removal_service:upload:#{registry.file_id}",
+            timeout: Geo::FileRegistryRemovalService::LEASE_TIMEOUT)
+        end
+
+        it 'deletes registry entry' do
+          expect do
+            service.execute
+          end.to change(Geo::UploadRegistry, :count).by(-1)
+        end
+      end
+
+      let!(:upload) { create(:upload, :with_file) }
       let!(:registry) { create(:geo_upload_registry, file_id: upload.id) }
       let!(:file_path) { upload.retrieve_uploader.file.path }
 
-      subject(:service) { described_class.new('upload', registry.file_id) }
+      it_behaves_like 'removes upload'
 
-      before do
-        stub_exclusive_lease("file_registry_removal_service:upload:#{registry.file_id}",
-          timeout: Geo::FileRegistryRemovalService::LEASE_TIMEOUT)
-      end
+      context 'migrated to object storage' do
+        before do
+          upload.update_column(:store, ObjectStorage::Store::REMOTE)
+        end
+
+        context 'with object storage enabled' do
+          before do
+            stub_uploads_object_storage(AvatarUploader)
+          end
+
+          it_behaves_like 'removes upload'
+        end
+
+        context 'with object storage disabled' do
+          before do
+            stub_uploads_object_storage(AvatarUploader, enabled: false)
+          end
 
-      it 'file from disk' do
-        expect do
-          service.execute
-        end.to change { File.exist?(file_path) }.from(true).to(false)
+          it_behaves_like 'removes upload registry'
+        end
       end
 
-      it 'deletes registry entry' do
-        expect do
-          service.execute
-        end.to change(Geo::UploadRegistry, :count).by(-1)
+      context 'no upload record' do
+        before do
+          upload.delete
+        end
+
+        it_behaves_like 'removes upload' do
+          subject(:service) { described_class.new('upload', registry.file_id, file_path) }
+        end
+
+        it_behaves_like 'removes upload registry'
       end
     end
   end