Geo: Refactor verification related concerns

- Create VerifiableModel concern to break down
VerificationState concern such that
VerifiableModel holds common code to be used for
verification on both primary and secondary, which
VerificationState is specifically for checksumming on primary.

.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