Merge branch 'mk/refactor-by-introducing-replicator-param' into 'master'

Geo: Use more consistent keyword args for Replicators

Closes #218592

See merge request gitlab-org/gitlab!31049

ee/app/models/geo_node.rb

@@ -179,7 +179,7 @@ class GeoNode < ApplicationRecord
   #
   # @param [String] replicable_name
   # @param [Integer] replicable_id
-  def geo_retrieve_url(replicable_name, replicable_id)
+  def geo_retrieve_url(replicable_name:, replicable_id:)
     geo_api_url("retrieve/#{replicable_name}/#{replicable_id}")
   end
 

ee/app/services/geo/blob_upload_service.rb

@@ -2,15 +2,12 @@
 
 module Geo
   class BlobUploadService
-    attr_reader :replicable_name, :blob_id, :checksum, :replicator
+    attr_reader :checksum, :replicator
 
-    def initialize(replicable_name:, blob_id:, decoded_params:)
-      @replicable_name = replicable_name
-      @blob_id = blob_id
+    def initialize(replicable_name:, replicable_id:, decoded_params:)
       @checksum = decoded_params.delete(:checksum)
 
-      replicator_klass = Gitlab::Geo::Replicator.for_replicable_name(replicable_name)
-      @replicator = replicator_klass.new(model_record_id: blob_id)
+      @replicator = Gitlab::Geo::Replicator.for_replicable_params(replicable_name: replicable_name, replicable_id: replicable_id)
     end
 
     def execute

ee/app/services/geo/event_service.rb

@@ -24,8 +24,7 @@ module Geo
       strong_memoize(:replicator) do
         model_record_id = payload[:model_record_id]
 
-        replicator_class = ::Gitlab::Geo::Replicator.for_replicable_name(replicable_name)
-        replicator_class.new(model_record_id: model_record_id)
+        ::Gitlab::Geo::Replicator.for_replicable_params(replicable_name: replicable_name, replicable_id: model_record_id)
       end
     end
 

ee/app/workers/geo/blob_verification_primary_worker.rb

@@ -11,8 +11,7 @@ module Geo
     idempotent!
 
     def perform(replicable_name, replicable_id)
-      replicator_class = ::Gitlab::Geo::Replicator.for_replicable_name(replicable_name)
-      replicator = replicator_class.new(model_record_id: replicable_id)
+      replicator = ::Gitlab::Geo::Replicator.for_replicable_params(replicable_name: replicable_name, replicable_id: replicable_id)
 
       replicator.calculate_checksum!
     rescue ActiveRecord::RecordNotFound

ee/lib/api/geo.rb

@@ -22,16 +22,15 @@ module API
 
       params do
         requires :replicable_name, type: String, desc: 'Replicable name (eg. package_file)'
-        requires :id, type: Integer, desc: 'The model ID that needs to be transferred'
+        requires :replicable_id, type: Integer, desc: 'The replicable ID that needs to be transferred'
       end
-      get 'retrieve/:replicable_name/:id' do
+      get 'retrieve/:replicable_name/:replicable_id' do
         check_gitlab_geo_request_ip!
-        authorize_geo_transfer!(replicable_name: params[:replicable_name], id: params[:id])
+        params_sym = params.symbolize_keys
+        authorize_geo_transfer!(params_sym)
 
         decoded_params = geo_jwt_decoder.decode
-        service = ::Geo::BlobUploadService.new(replicable_name: params[:replicable_name],
-                                               blob_id: params[:id],
-                                               decoded_params: decoded_params)
+        service = ::Geo::BlobUploadService.new(**params_sym, decoded_params: decoded_params)
         response = service.execute
 
         if response[:code] == :ok

ee/lib/gitlab/geo/replication/blob_downloader.rb

@@ -8,7 +8,7 @@ module Gitlab
 
         attr_reader :replicator
 
-        delegate :replicable_name, :model_record, :primary_checksum, :carrierwave_uploader, to: :replicator
+        delegate :primary_checksum, :carrierwave_uploader, to: :replicator
         delegate :file_storage?, to: :carrierwave_uploader
 
         class Result
@@ -49,7 +49,7 @@ module Gitlab
 
         # @return [String] URL to download the resource from
         def resource_url
-          Gitlab::Geo.primary_node.geo_retrieve_url(replicable_name, model_record.id.to_s)
+          Gitlab::Geo.primary_node.geo_retrieve_url(**replicator.replicable_params)
         end
 
         private
@@ -62,10 +62,7 @@ module Gitlab
         #
         # @return [Hash] HTTP request headers
         def request_headers
-          request_data = {
-            replicable_name: replicable_name,
-            id: model_record.id
-          }
+          request_data = replicator.replicable_params
 
           TransferRequest.new(request_data).headers
         end
@@ -181,7 +178,7 @@ module Gitlab
             pathname = Pathname.new(absolute_path)
             temp = Tempfile.new(TEMP_PREFIX, pathname.dirname.to_s)
           else
-            temp = Tempfile.new("#{TEMP_PREFIX}-#{replicable_name}-#{model_record.id}")
+            temp = Tempfile.new("#{TEMP_PREFIX}-#{replicator.replicable_name}-#{replicator.model_record_id}")
           end
 
           temp.chmod(default_permissions)

ee/lib/gitlab/geo/replicator.rb

@@ -92,6 +92,18 @@ module Gitlab
         raise e
       end
 
+      # Given the output of a replicator's `replicable_params`, reinstantiate
+      # the replicator instance
+      #
+      # @param [String] replicable_name of a replicator instance
+      # @param [Integer] replicable_id of a replicator instance
+      # @return [Geo::Replicator] replicator instance
+      def self.for_replicable_params(replicable_name:, replicable_id:)
+        replicator_class = for_replicable_name(replicable_name)
+
+        replicator_class.new(model_record_id: replicable_id)
+      end
+
       def self.checksummed
         model.checksummed
       end
@@ -127,7 +139,7 @@ module Gitlab
       # @param [Integer] model_record_id
       def initialize(model_record: nil, model_record_id: nil)
         @model_record = model_record
-        @model_record_id = model_record_id
+        @model_record_id = model_record_id || model_record&.id
       end
 
       # Instance of the replicable model
@@ -233,6 +245,14 @@ module Gitlab
         end
       end
 
+      # Return exactly the data needed by `for_replicable_params` to
+      # reinstantiate this Replicator elsewhere.
+      #
+      # @return [Hash] the replicable name and ID
+      def replicable_params
+        { replicable_name: replicable_name, replicable_id: model_record_id }
+      end
+
       protected
 
       # Store an event on the database

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

@@ -30,6 +30,10 @@ describe Gitlab::Geo::Replicator do
         event :test
         event :another_test
 
+        def self.model
+          ::DummyModel
+        end
+
         protected
 
         def consume_event_test(user:, other:)
@@ -224,7 +228,7 @@ describe Gitlab::Geo::Replicator do
 
       # We cannot infer parent project, so parent_project_id should be overridden.
       context 'when model_record does not respond to project_id' do
-        let(:model_record) { double(:model_record) }
+        let(:model_record) { double(:model_record, id: 555) }
 
         it 'raises NotImplementedError' do
           expect { replicator.parent_project_id }.to raise_error(NotImplementedError)
@@ -233,12 +237,67 @@ describe Gitlab::Geo::Replicator do
 
       # We assume project_id to be the parent project.
       context 'when model_record responds to project_id' do
-        let(:model_record) { double(:model_record, project_id: 1234) }
+        let(:model_record) { double(:model_record, id: 555, project_id: 1234) }
 
         it 'does not error' do
           expect(replicator.parent_project_id).to eq(1234)
         end
       end
     end
+
+    describe '.for_replicable_params' do
+      it 'returns the corresponding Replicator instance' do
+        replicator = described_class.for_replicable_params(replicable_name: 'dummy', replicable_id: 123456)
+
+        expect(replicator).to be_a(Geo::DummyReplicator)
+        expect(replicator.model_record_id).to eq(123456)
+      end
+    end
+
+    describe '.replicable_params' do
+      it 'returns a Hash of data needed to reinstantiate the Replicator' do
+        replicator = Geo::DummyReplicator.new(model_record_id: 123456)
+
+        expect(replicator.replicable_params).to eq(replicable_name: 'dummy', replicable_id: 123456)
+      end
+    end
+
+    describe '#initialize' do
+      subject(:replicator) { Geo::DummyReplicator.new(**args) }
+
+      let(:model_record) { double('DummyModel instance', id: 1234) }
+
+      context 'given model_record' do
+        let(:args) { { model_record: model_record } }
+
+        it 'sets model_record' do
+          expect(replicator.model_record).to eq(model_record)
+        end
+
+        it 'sets model_record_id' do
+          expect(replicator.model_record_id).to eq(1234)
+        end
+      end
+
+      context 'given model_record_id' do
+        let(:args) { { model_record_id: 1234 } }
+
+        before do
+          model = double('DummyModel')
+          # These two stubs are needed because `#model_record` instantiates the
+          # defined `.model` class.
+          allow(Geo::DummyReplicator).to receive(:model).and_return(model)
+          allow(model).to receive(:find).with(1234).and_return(model_record)
+        end
+
+        it 'sets model_record' do
+          expect(replicator.model_record).to eq(model_record)
+        end
+
+        it 'sets model_record_id' do
+          expect(replicator.model_record_id).to eq(1234)
+        end
+      end
+    end
   end
 end

ee/spec/models/geo_node_spec.rb

@@ -536,6 +536,14 @@ describe GeoNode, :request_store, :geo, type: :model do
     end
   end
 
+  describe '#geo_retrieve_url' do
+    let(:retrieve_url) { "https://localhost:3000/gitlab/api/#{api_version}/geo/retrieve/package_file/1" }
+
+    it 'returns api url based on node uri' do
+      expect(new_node.geo_retrieve_url(replicable_name: :package_file, replicable_id: 1)).to eq(retrieve_url)
+    end
+  end
+
   describe '#geo_transfers_url' do
     let(:transfers_url) { "https://localhost:3000/gitlab/api/#{api_version}/geo/transfers/lfs/1" }
 

ee/spec/requests/api/geo_spec.rb

@@ -38,7 +38,7 @@ describe API::Geo do
     end
   end
 
-  describe 'GET /geo/retrieve/:replicable/:id' do
+  describe 'GET /geo/retrieve/:replicable_name/:replicable_id' do
     before do
       stub_current_geo_node(secondary_node)
     end
@@ -84,7 +84,7 @@ describe API::Geo do
       end
 
       it 'responds with 401 with mismatched params in auth headers' do
-        wrong_headers = Gitlab::Geo::TransferRequest.new({ replicable_name: 'wrong', id: 1234 }).headers
+        wrong_headers = Gitlab::Geo::TransferRequest.new({ replicable_name: 'wrong', replicable_id: 1234 }).headers
 
         get api("/geo/retrieve/#{replicator.replicable_name}/#{resource.id}"), headers: wrong_headers
 
@@ -92,7 +92,7 @@ describe API::Geo do
       end
 
       it 'responds with 404 when resource is not found' do
-        model_not_found_header = Gitlab::Geo::TransferRequest.new({ replicable_name: replicator.replicable_name, id: 1234 }).headers
+        model_not_found_header = Gitlab::Geo::TransferRequest.new({ replicable_name: replicator.replicable_name, replicable_id: 1234 }).headers
 
         get api("/geo/retrieve/#{replicator.replicable_name}/1234"), headers: model_not_found_header
 

ee/spec/services/geo/blob_upload_service_spec.rb

@@ -5,7 +5,7 @@ require 'spec_helper'
 describe Geo::BlobUploadService do
   let(:package_file) { create(:package_file, :npm) }
 
-  subject { described_class.new(replicable_name: 'package_file', blob_id: package_file.id, decoded_params: {}) }
+  subject { described_class.new(replicable_name: 'package_file', replicable_id: package_file.id, decoded_params: {}) }
 
   describe '#initialize' do
     it 'initializes with valid attributes' do
@@ -19,7 +19,7 @@ describe Geo::BlobUploadService do
     end
 
     it 'errors with an invalid attributes' do
-      service = described_class.new(replicable_name: 'package_file', blob_id: 1234567890, decoded_params: {})
+      service = described_class.new(replicable_name: 'package_file', replicable_id: non_existing_record_id, decoded_params: {})
 
       response = service.execute
 
@@ -27,7 +27,7 @@ describe Geo::BlobUploadService do
     end
 
     it 'returns a file with valid attributes' do
-      service = described_class.new(replicable_name: 'package_file', blob_id: package_file.id,
+      service = described_class.new(replicable_name: 'package_file', replicable_id: package_file.id,
                                     decoded_params: { checksum: package_file.verification_checksum })
 
       response = service.execute

ee/spec/services/geo/event_service_spec.rb

@@ -13,7 +13,7 @@ describe Geo::EventService do
 
   describe '#execute' do
     before do
-      resource_url = primary.geo_retrieve_url('package_file', model_record.id.to_s)
+      resource_url = primary.geo_retrieve_url(replicable_name: 'package_file', replicable_id: model_record.id.to_s)
       content = model_record.file.open
       File.unlink(model_record.file.path)
       stub_request(:get, resource_url).to_return(status: 200, body: content)