Merge branch 'mk/fix-consuming-blob-events' into 'master'

Geo: Fix consuming events of blobs that should not be synced

Closes #224634

See merge request gitlab-org/gitlab!38089

ee/app/models/concerns/geo/blob_replicator_strategy.rb

@@ -21,7 +21,7 @@ module Geo
 
     # Called by Gitlab::Geo::Replicator#consume
     def consume_event_created(**params)
-      return if excluded_by_selective_sync?
+      return unless in_replicables_for_geo_node?
 
       download
     end
@@ -34,7 +34,7 @@ module Geo
 
     # Called by Gitlab::Geo::Replicator#consume
     def consume_event_deleted(**params)
-      return if excluded_by_selective_sync?
+      return unless in_replicables_for_geo_node?
 
       replicate_destroy(params)
     end

ee/changelogs/unreleased/mk-fix-consuming-blob-events.yml

+---
+title: 'Geo: Fix consuming events of blobs that should not be synced'
+merge_request: 38089
+author:
+type: fixed

ee/lib/gitlab/geo/replicable_model.rb

@@ -82,6 +82,10 @@ module Gitlab
           replicator.carrierwave_uploader.exists?
         end
       end
+
+      def in_replicables_for_geo_node?
+        self.class.replicables_for_geo_node.id_in(self).exists?
+      end
     end
   end
 end

ee/lib/gitlab/geo/replicator.rb

@@ -19,6 +19,7 @@ module Gitlab
       attr_reader :model_record_id
 
       delegate :model, to: :class
+      delegate :in_replicables_for_geo_node?, to: :model_record
 
       class << self
         delegate :find_unsynced_registries, :find_failed_registries, to: :registry_class
@@ -275,26 +276,6 @@ module Gitlab
         registry.verification_checksum
       end
 
-      # This method does not yet cover resources that are owned by a namespace
-      # but not a project, because we do not have that use-case...yet.
-      # E.g. GroupWikis will need it.
-      def excluded_by_selective_sync?
-        # If the replicable is not owned by a project or namespace, then selective sync cannot apply to it.
-        return false unless parent_project_id
-
-        !current_node.projects_include?(parent_project_id)
-      end
-
-      def parent_project_id
-        strong_memoize(:parent_project_id) do
-          # We should never see this at runtime. All Replicators should be tested
-          # by `it_behaves_like 'a replicator'`, which would reveal this problem.
-          selective_sync_not_implemented_error(__method__) unless model_record.respond_to?(:project_id)
-
-          model_record.project_id
-        end
-      end
-
       # Return exactly the data needed by `for_replicable_params` to
       # reinstantiate this Replicator elsewhere.
       #
@@ -335,11 +316,6 @@ module Gitlab
       def current_node
         Gitlab::Geo.current_node
       end
-
-      def selective_sync_not_implemented_error(method_name)
-        raise NotImplementedError,
-            "#{self.class} does not implement #{method_name}. If selective sync is not applicable, just return nil."
-      end
     end
   end
 end

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

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+# Also see ee/spec/support/shared_examples/models/concerns/replicable_model_shared_examples.rb:
+#
+# - Place tests here in replicable_model_spec.rb if you want to run them once,
+#   against a DummyModel.
+# - Place tests in replicable_model_shared_examples.rb if you want them to be
+#   run against every real Model.
+RSpec.describe Gitlab::Geo::ReplicableModel do
+  include ::EE::GeoHelpers
+
+  let_it_be(:primary_node) { create(:geo_node, :primary) }
+  let_it_be(:secondary_node) { create(:geo_node) }
+
+  before(:all) do
+    create_dummy_model_table
+  end
+
+  after(:all) do
+    drop_dummy_model_table
+  end
+
+  before do
+    stub_dummy_replicator_class
+    stub_dummy_model_class
+  end
+
+  subject { DummyModel.new }
+
+  it_behaves_like 'a replicable model' do
+    let(:model_record) { subject }
+  end
+
+  describe '#replicator' do
+    it 'adds replicator method to the model' do
+      expect(subject).to respond_to(:replicator)
+    end
+
+    it 'instantiates a replicator into the model' do
+      expect(subject.replicator).to be_a(Geo::DummyReplicator)
+    end
+  end
+
+  describe '#in_replicables_for_geo_node?' do
+    it 'reuses replicables_for_geo_node' do
+      expect(DummyModel).to receive(:replicables_for_geo_node).once.and_return(DummyModel.all)
+
+      subject.in_replicables_for_geo_node?
+    end
+  end
+end

ee/spec/support/helpers/ee/geo_helpers.rb

@@ -55,5 +55,67 @@ module EE
       # will appear as though the tracking DB were not available
       allow(::Gitlab::Geo).to receive(:geo_database_configured?).and_call_original
     end
+
+    def stub_dummy_replicator_class
+      stub_const('Geo::DummyReplicator', Class.new(::Gitlab::Geo::Replicator))
+
+      Geo::DummyReplicator.class_eval do
+        event :test
+        event :another_test
+
+        def self.model
+          ::DummyModel
+        end
+
+        def handle_after_create_commit
+          true
+        end
+
+        protected
+
+        def consume_event_test(user:, other:)
+          true
+        end
+      end
+    end
+
+    def stub_dummy_model_class
+      stub_const('DummyModel', Class.new(ApplicationRecord))
+
+      DummyModel.class_eval do
+        include ::Gitlab::Geo::ReplicableModel
+
+        with_replicator Geo::DummyReplicator
+
+        def self.replicables_for_geo_node
+          self.all
+        end
+      end
+
+      DummyModel.reset_column_information
+    end
+
+    # Example:
+    #
+    # before(:all) do
+    #   create_dummy_model_table
+    # end
+    #
+    # after(:all) do
+    #   drop_dummy_model_table
+    # end
+    def create_dummy_model_table
+      ActiveRecord::Schema.define do
+        create_table :dummy_models, force: true do |t|
+          t.binary :verification_checksum
+        end
+      end
+    end
+
+    def drop_dummy_model_table
+      ActiveRecord::Schema.define do
+        drop_table :dummy_models, force: true
+      end
+    end
   end
 end

ee/spec/support/shared_examples/models/concerns/blob_replicator_strategy_shared_examples.rb

@@ -20,6 +20,9 @@ RSpec.shared_examples 'a blob replicator' do
 
   it_behaves_like 'a replicator'
 
+  # This could be included in each model's spec, but including it here is DRYer.
+  include_examples 'a replicable model'
+
   describe '#handle_after_create_commit' do
     it 'creates a Geo::Event' do
       expect do
@@ -104,9 +107,9 @@ RSpec.shared_examples 'a blob replicator' do
   end
 
   describe '#consume_event_created' do
-    context "when the blob's project is not excluded by selective sync" do
+    context "when the blob's project is in replicables for this geo node" do
       it 'invokes Geo::BlobDownloadService' do
-        expect(replicator).to receive(:excluded_by_selective_sync?).and_return(false)
+        expect(replicator).to receive(:in_replicables_for_geo_node?).and_return(true)
         service = double(:service)
 
         expect(service).to receive(:execute)
@@ -116,9 +119,9 @@ RSpec.shared_examples 'a blob replicator' do
       end
     end
 
-    context "when the blob's project is excluded by selective sync" do
+    context "when the blob's project is not in replicables for this geo node" do
       it 'does not invoke Geo::BlobDownloadService' do
-        expect(replicator).to receive(:excluded_by_selective_sync?).and_return(true)
+        expect(replicator).to receive(:in_replicables_for_geo_node?).and_return(false)
 
         expect(::Geo::BlobDownloadService).not_to receive(:new)
 
@@ -128,9 +131,9 @@ RSpec.shared_examples 'a blob replicator' do
   end
 
   describe '#consume_event_deleted' do
-    context "when the blob's project is not excluded by selective sync" do
+    context "when the blob's project is in replicables for this geo node" do
       it 'invokes Geo::FileRegistryRemovalService' do
-        expect(replicator).to receive(:excluded_by_selective_sync?).and_return(false)
+        expect(replicator).to receive(:in_replicables_for_geo_node?).and_return(true)
         service = double(:service)
 
         expect(service).to receive(:execute)
@@ -141,9 +144,9 @@ RSpec.shared_examples 'a blob replicator' do
       end
     end
 
-    context "when the blob's project is excluded by selective sync" do
+    context "when the blob's project is not in replicables for this geo node" do
       it 'does not invoke Geo::FileRegistryRemovalService' do
-        expect(replicator).to receive(:excluded_by_selective_sync?).and_return(true)
+        expect(replicator).to receive(:in_replicables_for_geo_node?).and_return(false)
 
         expect(::Geo::FileRegistryRemovalService).not_to receive(:new)
 
@@ -172,18 +175,5 @@ RSpec.shared_examples 'a blob replicator' do
     it 'is a Class' do
       expect(invoke_model).to be_a(Class)
     end
-
-    # For convenience (and reliability), instead of asking developers to include shared examples on each model spec as well
-    context 'replicable model' do
-      it 'defines #replicator' do
-        expect(model_record).to respond_to(:replicator)
-      end
-
-      it 'invokes replicator.handle_after_create_commit on create' do
-        expect(replicator).to receive(:handle_after_create_commit)
-
-        model_record.save!
-      end
-    end
   end
 end

ee/spec/support/shared_examples/models/concerns/replicable_model_shared_examples.rb

+# frozen_string_literal: true
+
+# Required let variables:
+#
+# - model_record: A valid, unpersisted instance of the model class
+#
+# We do not use `described_class` here, so we can include this in replicator
+# strategy shared examples instead of in *every* model spec.
+#
+# Also see ee/spec/lib/gitlab/geo/replicable_model_spec.rb:
+#
+# - Place tests in replicable_model_spec.rb if you want to run them once,
+#   against a DummyModel.
+# - Place tests here in replicable_model_shared_examples.rb if you want them to
+#   be run against every real Model.
+RSpec.shared_examples 'a replicable model' do
+  include EE::GeoHelpers
+
+  describe '#replicator' do
+    it 'is defined and does not raise error' do
+      expect(model_record.replicator).to be_a(Gitlab::Geo::Replicator)
+    end
+  end
+
+  it 'invokes replicator.handle_after_create_commit on create' do
+    expect(model_record.replicator).to receive(:handle_after_create_commit)
+
+    model_record.save!
+  end
+
+  describe '.replicables_for_geo_node' do
+    let_it_be(:secondary) { create(:geo_node) }
+
+    before do
+      stub_current_geo_node(secondary)
+    end
+
+    it 'is implemented' do
+      expect(model_record.class.replicables_for_geo_node).to be_an(ActiveRecord::Relation)
+    end
+  end
+end

ee/spec/support/shared_examples/models/concerns/replicator_shared_examples.rb

@@ -13,12 +13,6 @@
 RSpec.shared_examples 'a replicator' do
   include EE::GeoHelpers
 
-  describe '#parent_project_id' do
-    it 'is implemented if needed' do
-      expect { replicator.parent_project_id }.not_to raise_error
-    end
-  end
-
   context 'Geo node status' do
     context 'on a secondary node' do
       let_it_be(:registry_factory) { registry_factory_name(described_class.registry_class) }