Merge branch 'mk/generalize-replicable-name-not-implemented-error' into 'master'

Geo: `Replicator.for_replicable_name` should raise (and log) NotImplementedError

Closes #217029

See merge request gitlab-org/gitlab!31388

ee/app/services/geo/blob_upload_service.rb

@@ -3,7 +3,6 @@
 module Geo
   class BlobUploadService
     include ExclusiveLeaseGuard
-    include ::Gitlab::Geo::LogHelpers
 
     attr_reader :replicable_name, :blob_id, :checksum, :replicator
 
@@ -14,8 +13,6 @@ module Geo
 
       replicator_klass = Gitlab::Geo::Replicator.for_replicable_name(replicable_name)
       @replicator = replicator_klass.new(model_record_id: blob_id)
-
-      fail_unimplemented!(replicable_name) unless @replicator
     end
 
     def execute
@@ -25,25 +22,5 @@ module Geo
     def retriever
       Gitlab::Geo::Replication::BlobRetriever.new(replicator: replicator, checksum: checksum)
     end
-
-    private
-
-    def fail_unimplemented!(replicable_name)
-      error_message = "Cannot find a Geo::Replicator for #{replicable_name}"
-
-      log_error(error_message)
-
-      raise NotImplementedError, error_message
-    end
-
-    # This is called by LogHelpers to build json log with context info
-    #
-    # @see ::Gitlab::Geo::LogHelpers
-    def extra_log_data
-      {
-        replicable_name: replicable_name,
-        blob_id: blob_id
-      }.compact
-    end
   end
 end

ee/lib/gitlab/geo/replicator.rb

@@ -11,6 +11,7 @@ module Gitlab
     # Each replicator is tied to a specific replicable resource
     class Replicator
       include ::Gitlab::Geo::LogHelpers
+      extend ::Gitlab::Geo::LogHelpers
 
       CLASS_SUFFIXES = %w(RegistryFinder RegistriesResolver).freeze
 
@@ -73,7 +74,7 @@ module Gitlab
         const_get("::Types::Geo::#{replicable_name.camelize}RegistryType", false)
       end
 
-      # Given a `replicable_name`, return the corresponding replicator
+      # Given a `replicable_name`, return the corresponding replicator class
       #
       # @param [String] replicable_name the replicable slug
       # @return [Class<Geo::Replicator>] replicator implementation
@@ -81,6 +82,13 @@ module Gitlab
         replicator_class_name = "::Geo::#{replicable_name.camelize}Replicator"
 
         const_get(replicator_class_name, false)
+      rescue NameError
+        message = "Cannot find a Geo::Replicator for #{replicable_name}"
+        e = NotImplementedError.new(message)
+
+        log_error(message, e, { replicable_name: replicable_name })
+
+        raise e
       end
 
       def self.checksummed

ee/spec/lib/gitlab/geo/replicator_spec.rb

@@ -150,5 +150,33 @@ describe Gitlab::Geo::Replicator do
         end
       end
     end
+
+    describe '.for_replicable_name' do
+      context 'given a valid replicable_name' do
+        it 'returns the corresponding Replicator class' do
+          replicator_class = described_class.for_replicable_name('dummy')
+
+          expect(replicator_class).to eq(Geo::DummyReplicator)
+        end
+      end
+
+      context 'given an invalid replicable_name' do
+        it 'raises and logs NotImplementedError' do
+          expect(Gitlab::Geo::Logger).to receive(:error)
+
+          expect do
+            described_class.for_replicable_name('invalid')
+          end.to raise_error(NotImplementedError)
+        end
+      end
+
+      context 'given nil' do
+        it 'raises NotImplementedError' do
+          expect do
+            described_class.for_replicable_name('invalid')
+          end.to raise_error(NotImplementedError)
+        end
+      end
+    end
   end
 end