Merge branch 'ag-re-factor-scope-replicable-model' into 'master'

Geo: Refactor verification related concerns

See merge request gitlab-org/gitlab!77257

.gitlab/issue_templates/Geo Replicate a new Git repository type.md

@@ -175,8 +175,8 @@ That's all of the required database changes.
 #### Step 1. Implement replication and verification
 
 - [ ] Add the following lines to the `cool_widget` model to accomplish some important tasks:
-  - Include `Gitlab::Geo::ReplicableModel` in the `CoolWidget` class, and specify the Replicator class `with_replicator Geo::CoolWidgetReplicator`.
-  - Include the `::Gitlab::Geo::VerificationState` concern.
+  - Include `::Geo::ReplicableModel` in the `CoolWidget` class, and specify the Replicator class `with_replicator Geo::CoolWidgetReplicator`.
+  - Include the `::Geo::VerifiableModel` concern.
   - Delegate verification related methods to the `cool_widget_state` model.
   - For verification, override some scopes to use the `cool_widget_states` table instead of the model table.
   - Implement the `verification_state_object` method to return the object that holds
@@ -192,8 +192,8 @@ That's all of the required database changes.
 
   class CoolWidget < ApplicationRecord
     ...
-    include ::Gitlab::Geo::ReplicableModel
-    include ::Gitlab::Geo::VerificationState
+    include ::Geo::ReplicableModel
+    include ::Geo::VerifiableModel
 
     with_replicator Geo::CoolWidgetReplicator
 

.gitlab/issue_templates/Geo Replicate a new blob type.md

@@ -179,8 +179,8 @@ That's all of the required database changes.
 #### Step 1. Implement replication and verification
 
 - [ ] Add the following lines to the `cool_widget` model to accomplish some important tasks:
-  - Include `Gitlab::Geo::ReplicableModel` in the `CoolWidget` class, and specify the Replicator class `with_replicator Geo::CoolWidgetReplicator`.
-  - Include the `::Gitlab::Geo::VerificationState` concern.
+  - Include `::Geo::ReplicableModel` in the `CoolWidget` class, and specify the Replicator class `with_replicator Geo::CoolWidgetReplicator`.
+  - Include the `::Geo::VerifiableModel` concern.
   - Delegate verification related methods to the `cool_widget_state` model.
   - For verification, override some scopes to use the `cool_widget_states` table instead of the model table.
   - Implement the `verification_state_object` method to return the object that holds
@@ -194,8 +194,8 @@ That's all of the required database changes.
 
   class CoolWidget < ApplicationRecord
     ...
-    include ::Gitlab::Geo::ReplicableModel
-    include ::Gitlab::Geo::VerificationState
+    include ::Geo::ReplicableModel
+    include ::Geo::VerifiableModel
 
     with_replicator Geo::CoolWidgetReplicator
 

doc/development/geo/framework.md

@@ -117,7 +117,7 @@ the model code:
 
 ```ruby
 class Packages::PackageFile < ApplicationRecord
-  include ::Gitlab::Geo::ReplicableModel
+  include ::Geo::ReplicableModel
 
   with_replicator Geo::PackageFileReplicator
 end

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

+# frozen_string_literal: true
+
+module Geo
+  module ReplicableModel
+    extend ActiveSupport::Concern
+    include Checksummable
+
+    included do
+      # If this hook turns out not to apply to all Models, perhaps we should extract a `ReplicableBlobModel`
+      after_create_commit -> { replicator.handle_after_create_commit if replicator.respond_to?(:handle_after_create_commit) }
+      after_destroy -> { replicator.handle_after_destroy if replicator.respond_to?(:handle_after_destroy) }
+
+      # Temporarily defining `verification_succeeded` and
+      # `verification_failed` for unverified models while verification is
+      # under development to avoid breaking GeoNodeStatusCheck code.
+      # TODO: Remove these after including `::Geo::VerificationState` on
+      # all models. https://gitlab.com/gitlab-org/gitlab/-/issues/280768
+      scope :verification_succeeded, -> { none }
+      scope :verification_failed, -> { none }
+
+      # These scopes are intended to be overridden as needed
+      scope :available_replicables, -> { all }
+
+      # On primary, `verifiables` are records that can be checksummed and/or are replicable.
+
+      # On secondary, `verifiables` are records that have already been replicated
+      # and (ideally) have been checksummed on the primary
+
+      scope :verifiables, -> { self.respond_to?(:with_files_stored_locally) ? available_replicables.with_files_stored_locally : available_replicables }
+
+      # When storing verification details in the same table as the model,
+      # the scope `available_verifiables` returns only those records
+      # that are eligible for verification, i.e. the same as the scope
+      # `verifiables`.
+
+      # When using a separate table to store verification details,
+      # the scope `available_verifiables` should return all records
+      # from the separate table because the separate table will
+      # always only have records corresponding to replicables that are verifiable.
+      # For this, override the scope in the replicable model, e.g. like so in
+      # `MergeRequestDiff`,
+      # `scope :available_verifiables, -> { joins(:merge_request_diff_detail) }`
+
+      scope :available_verifiables, -> { verifiables }
+    end
+
+    class_methods do
+      # Associate current model with specified replicator
+      #
+      # @param [Gitlab::Geo::Replicator] klass
+      def with_replicator(klass)
+        raise ArgumentError, 'Must be a class inheriting from Gitlab::Geo::Replicator' unless klass < ::Gitlab::Geo::Replicator
+
+        class_eval <<-RUBY, __FILE__, __LINE__ + 1
+          define_method :replicator do
+            @_replicator ||= klass.new(model_record: self)
+          end
+
+          define_singleton_method :replicator_class do
+            @_replicator_class ||= klass
+          end
+        RUBY
+      end
+    end
+
+    # Geo Replicator
+    #
+    # @abstract
+    # @return [Gitlab::Geo::Replicator]
+    def replicator
+      raise NotImplementedError, 'There is no Replicator defined for this model'
+    end
+
+    def in_replicables_for_current_secondary?
+      self.class.replicables_for_current_secondary(self).exists?
+    end
+  end
+end

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

+# frozen_string_literal: true
+
+module Geo
+  # This concern is included on Model classes (as opposed to Registry classes)
+  # to manage their verification states. Note that this concern does not handle
+  # how verification is performed; see `VerifiableReplicator`.
+  #
+  # It handles both cases where verification state is stored in a separate
+  # table or when it is stored in the same table as the model.
+
+  module VerifiableModel
+    extend ActiveSupport::Concern
+    include ::Geo::VerificationState
+
+    included do
+      def save_verification_details
+        return unless self.class.separate_verification_state_table?
+
+        return unless self.class.verifiables.primary_key_in(self).exists?
+
+        # During a transaction, `verification_state_object` could be built before
+        # a value for `verification_state_model_key` exists. So we check for that
+        # before saving the `verification_state_object`
+        unless verification_state_object.persisted?
+          verification_state_object[self.class.verification_state_model_key] = self.id
+        end
+
+        verification_state_object.save!
+      end
+
+      # Implement this method in the class that includes this concern to specify
+      # a different ActiveRecord association name that stores the verification state
+      # See module EE::MergeRequestDiff for example
+      def verification_state_object
+        raise NotImplementedError if self.class.separate_verification_state_table?
+
+        self
+      end
+    end
+
+    class_methods do
+      include Delay
+
+      def pluck_verification_details_ids_in_range(range)
+        verification_state_table_class
+          .where(self.verification_state_model_key => range)
+          .pluck(self.verification_state_model_key)
+      end
+
+      def pluck_verifiable_ids_in_range(range)
+        self
+          .verifiables
+          .primary_key_in(range)
+          .pluck_primary_key
+      end
+
+      # @return whether primary checksum data is stored in a table separate
+      #         from the model table
+      def separate_verification_state_table?
+        verification_state_table_name != table_name
+      end
+    end
+  end
+end

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

@@ -4,13 +4,13 @@ module Geo
   module VerifiableRegistry
     extend ActiveSupport::Concern
     extend ::Gitlab::Utils::Override
-    include ::Gitlab::Geo::VerificationState
+    include ::Geo::VerificationState
 
     class_methods do
       extend ::Gitlab::Utils::Override
 
-      # Overrides a method in `Gitlab::Geo::VerificationState`. This method is
-      # used by `Gitlab::Geo::VerificationState.start_verification_batch` to
+      # Overrides a method in `::Geo::VerificationState`. This method is
+      # used by `::Geo::VerificationState.start_verification_batch` to
       # produce a query which must return values of the primary key of the
       # *model*, not of the *registry*. We need this so we can instantiate
       # Replicators.

ee/app/models/ee/ci/pipeline_artifact.rb

@@ -6,8 +6,8 @@ module EE
       extend ActiveSupport::Concern
 
       prepended do
-        include ::Gitlab::Geo::ReplicableModel
-        include ::Gitlab::Geo::VerificationState
+        include ::Geo::ReplicableModel
+        include ::Geo::VerifiableModel
 
         with_replicator ::Geo::PipelineArtifactReplicator
       end

ee/app/models/ee/lfs_object.rb

@@ -11,8 +11,8 @@ module EE
     STORE_COLUMN = :file_store
 
     prepended do
-      include ::Gitlab::Geo::ReplicableModel
-      include ::Gitlab::Geo::VerificationState
+      include ::Geo::ReplicableModel
+      include ::Geo::VerifiableModel
 
       with_replicator ::Geo::LfsObjectReplicator
 

ee/app/models/ee/merge_request_diff.rb

@@ -5,9 +5,9 @@ module EE
     extend ActiveSupport::Concern
 
     prepended do
-      include ::Gitlab::Geo::ReplicableModel
+      include ::Geo::ReplicableModel
       include ObjectStorable
-      include ::Gitlab::Geo::VerificationState
+      include ::Geo::VerifiableModel
 
       STORE_COLUMN = :external_diff_store
 

ee/app/models/ee/packages/package_file.rb

@@ -6,8 +6,8 @@ module EE
       extend ActiveSupport::Concern
 
       prepended do
-        include ::Gitlab::Geo::ReplicableModel
-        include ::Gitlab::Geo::VerificationState
+        include ::Geo::ReplicableModel
+        include ::Geo::VerifiableModel
 
         with_replicator ::Geo::PackageFileReplicator
       end

ee/app/models/ee/pages_deployment.rb

@@ -5,8 +5,8 @@ module EE
     extend ActiveSupport::Concern
 
     prepended do
-      include ::Gitlab::Geo::ReplicableModel
-      include ::Gitlab::Geo::VerificationState
+      include ::Geo::ReplicableModel
+      include ::Geo::VerifiableModel
 
       with_replicator ::Geo::PagesDeploymentReplicator
 

ee/app/models/ee/snippet_repository.rb

@@ -5,8 +5,8 @@ module EE
     extend ActiveSupport::Concern
 
     prepended do
-      include ::Gitlab::Geo::ReplicableModel
-      include ::Gitlab::Geo::VerificationState
+      include ::Geo::ReplicableModel
+      include ::Geo::VerifiableModel
       include FromUnion
 
       with_replicator ::Geo::SnippetRepositoryReplicator

ee/app/models/ee/terraform/state_version.rb

@@ -6,8 +6,8 @@ module EE
       extend ActiveSupport::Concern
 
       prepended do
-        include ::Gitlab::Geo::ReplicableModel
-        include ::Gitlab::Geo::VerificationState
+        include ::Geo::ReplicableModel
+        include ::Geo::VerifiableModel
 
         with_replicator ::Geo::TerraformStateVersionReplicator
 

ee/app/models/ee/upload.rb

@@ -10,8 +10,8 @@ module EE
 
     prepended do
       include ::Gitlab::SQL::Pattern
-      include ::Gitlab::Geo::ReplicableModel
-      include ::Gitlab::Geo::VerificationState
+      include ::Geo::ReplicableModel
+      include ::Geo::VerifiableModel
 
       with_replicator ::Geo::UploadReplicator
 

ee/app/models/group_wiki_repository.rb

 # frozen_string_literal: true
 
 class GroupWikiRepository < ApplicationRecord
-  include ::Gitlab::Geo::ReplicableModel
+  include ::Geo::ReplicableModel
   include EachBatch
   include Shardable
 

ee/lib/gitlab/geo/replicable_model.rb

-# frozen_string_literal: true
-
-module Gitlab
-  module Geo
-    module ReplicableModel
-      extend ActiveSupport::Concern
-      include Checksummable
-
-      included do
-        # If this hook turns out not to apply to all Models, perhaps we should extract a `ReplicableBlobModel`
-        after_create_commit -> { replicator.handle_after_create_commit if replicator.respond_to?(:handle_after_create_commit) }
-        after_destroy -> { replicator.handle_after_destroy if replicator.respond_to?(:handle_after_destroy) }
-
-        # Temporarily defining `verification_succeeded` and
-        # `verification_failed` for unverified models while verification is
-        # under development to avoid breaking GeoNodeStatusCheck code.
-        # TODO: Remove these after including `Gitlab::Geo::VerificationState` on
-        # all models. https://gitlab.com/gitlab-org/gitlab/-/issues/280768
-        scope :verification_succeeded, -> { none }
-        scope :verification_failed, -> { none }
-
-        # These scopes are intended to be overridden as needed
-        scope :available_replicables, -> { all }
-
-        # On primary, `verifiables` are records that can be checksummed and/or are replicable.
-
-        # On secondary, `verifiables` are records that have already been replicated
-        # and (ideally) have been checksummed on the primary
-
-        scope :verifiables, -> { self.respond_to?(:with_files_stored_locally) ? available_replicables.with_files_stored_locally : available_replicables }
-
-        # When storing verification details in the same table as the model,
-        # the scope `available_verifiables` returns only those records
-        # that are eligible for verification, i.e. the same as the scope
-        # `verifiables`.
-
-        # When using a separate table to store verification details,
-        # the scope `available_verifiables` should return all records
-        # from the separate table because the separate table will
-        # always only have records corresponding to replicables that are verifiable.
-        # For this, override the scope in the replicable model, e.g. like so in
-        # `MergeRequestDiff`,
-        # `scope :available_verifiables, -> { joins(:merge_request_diff_detail) }`
-
-        scope :available_verifiables, -> { verifiables }
-      end
-
-      class_methods do
-        # Associate current model with specified replicator
-        #
-        # @param [Gitlab::Geo::Replicator] klass
-        def with_replicator(klass)
-          raise ArgumentError, 'Must be a class inheriting from Gitlab::Geo::Replicator' unless klass < ::Gitlab::Geo::Replicator
-
-          class_eval <<-RUBY, __FILE__, __LINE__ + 1
-            define_method :replicator do
-              @_replicator ||= klass.new(model_record: self)
-            end
-
-            define_singleton_method :replicator_class do
-              @_replicator_class ||= klass
-            end
-          RUBY
-        end
-      end
-
-      # Geo Replicator
-      #
-      # @abstract
-      # @return [Gitlab::Geo::Replicator]
-      def replicator
-        raise NotImplementedError, 'There is no Replicator defined for this model'
-      end
-
-      def in_replicables_for_current_secondary?
-        self.class.replicables_for_current_secondary(self).exists?
-      end
-    end
-  end
-end

ee/spec/lib/gitlab/geo/replicable_model_spec.rb → ee/spec/models/concerns/geo/replicable_model_spec.rb

@@ -8,7 +8,7 @@ require 'spec_helper'
 #   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
+RSpec.describe Geo::ReplicableModel do
   include ::EE::GeoHelpers
 
   let_it_be(:primary_node) { create(:geo_node, :primary) }
@@ -42,6 +42,17 @@ RSpec.describe Gitlab::Geo::ReplicableModel do
     it 'instantiates a replicator into the model' do
       expect(subject.replicator).to be_a(Geo::DummyReplicator)
     end
+
+    context 'when replicator is not defined in inheriting class' do
+      before do
+        stub_const('DummyModel', Class.new(ApplicationRecord))
+        DummyModel.class_eval { include ::Geo::ReplicableModel }
+      end
+
+      it 'raises NotImplementedError' do
+        expect { DummyModel.new.replicator }.to raise_error(NotImplementedError)
+      end
+    end
   end
 
   describe '#in_replicables_for_current_secondary?' do

ee/spec/models/concerns/geo/verifiable_model_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Geo::VerifiableModel do
+  include ::EE::GeoHelpers
+
+  context 'when separate table is used for verification state' do
+    before(:all) do
+      create_dummy_model_with_separate_state_table
+    end
+
+    after(:all) do
+      drop_dummy_model_with_separate_state_table
+    end
+
+    before do
+      stub_dummy_replicator_class(model_class: 'TestDummyModelWithSeparateState')
+      stub_dummy_model_with_separate_state_class
+    end
+
+    subject { TestDummyModelWithSeparateState.new }
+
+    describe '.verification_state_model_key' do
+      it 'returns the primary key of the state model' do
+        expect(subject.class.verification_state_model_key).to eq(TestDummyModelState.primary_key)
+      end
+    end
+  end
+
+  context 'when separate table is not used for verification state' do
+    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 }
+
+    describe '.verification_state_object' do
+      it 'returns self' do
+        expect(subject.verification_state_object.id).to eq(subject.id)
+      end
+    end
+
+    describe '.verification_state_model_key' do
+      it 'returns the primary key of the model' do
+        expect(subject.class.verification_state_model_key).to eq(DummyModel.primary_key)
+      end
+    end
+  end
+end

ee/spec/lib/gitlab/geo/verification_state_spec.rb → ee/spec/models/concerns/geo/verification_state_spec.rb

@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe Gitlab::Geo::VerificationState do
+RSpec.describe Geo::VerificationState do
   include ::EE::GeoHelpers
 
   let_it_be(:primary_node) { create(:geo_node, :primary) }
@@ -434,7 +434,7 @@ RSpec.describe Gitlab::Geo::VerificationState do
     end
 
     before do
-      stub_dummy_replicator_class(model_class: 'DummyModelWithSeparateState')
+      stub_dummy_replicator_class(model_class: 'TestDummyModelWithSeparateState')
       stub_dummy_model_with_separate_state_class
     end
 

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

@@ -79,8 +79,8 @@ module EE
       stub_const('DummyModel', Class.new(ApplicationRecord))
 
       DummyModel.class_eval do
-        include ::Gitlab::Geo::ReplicableModel
-        include ::Gitlab::Geo::VerificationState
+        include ::Geo::ReplicableModel
+        include ::Geo::VerifiableModel
 
         with_replicator Geo::DummyReplicator
 
@@ -170,8 +170,8 @@ module EE
       TestDummyModelWithSeparateState.class_eval do
         self.table_name = '_test_dummy_model_with_separate_states'
 
-        include ::Gitlab::Geo::ReplicableModel
-        include ::Gitlab::Geo::VerificationState
+        include ::Geo::ReplicableModel
+        include ::Geo::VerifiableModel
 
         with_replicator Geo::DummyReplicator