Save verification details conditionally

We want to store verification details only for
replicables that qualify for verification,
e.g. for MR diffs only externally stored diffs
are verified.

So, instead of autosaving child records, we
check for the same filters used in `available_verifiables`
and save the child object.

This commit also introduces a scope
`with_files_stored_locally` to limit verifiables
to locally stored files only, since verification
has not yet been implemented for remote object
storage.

EE: true
Changed: changed

ee/app/models/ee/merge_request_diff.rb

@@ -13,7 +13,9 @@ module EE
 
       with_replicator ::Geo::MergeRequestDiffReplicator
 
-      has_one :merge_request_diff_detail, autosave: true, inverse_of: :merge_request_diff
+      has_one :merge_request_diff_detail, autosave: false, inverse_of: :merge_request_diff
+
+      after_save :save_verification_details
 
       delegate :verification_retry_at, :verification_retry_at=,
                :verified_at, :verified_at=,
@@ -30,9 +32,10 @@ module EE
       scope :with_verification_state, ->(state) { joins(:merge_request_diff_detail).where(merge_request_diff_details: { verification_state: verification_state_value(state) }) }
       scope :checksummed, -> { joins(:merge_request_diff_detail).where.not(merge_request_diff_details: { verification_checksum: nil } ) }
       scope :not_checksummed, -> { joins(:merge_request_diff_detail).where(merge_request_diff_details: { verification_checksum: nil } ) }
+      scope :with_files_stored_locally, -> { where(klass::STORE_COLUMN => ::ObjectStorage::Store::LOCAL) }
 
-      def create_verification_details
-        create_merge_request_diff_detail
+      def verification_state_object
+        merge_request_diff_detail
       end
     end
 

ee/app/services/geo/verification_state_backfill_service.rb

@@ -66,7 +66,7 @@ module Geo
 
     def create_verification_details(range, for_creation_ids)
       replicable_model.find(for_creation_ids).map do |replicable|
-        replicable.create_verification_details
+        replicable.save_verification_details
       end
 
       log_created(range, for_creation_ids)

ee/lib/gitlab/geo/verification_state.rb

@@ -92,8 +92,28 @@ module Gitlab
           end
         end
 
-        def create_verification_details
-          raise NotImplementedError
+        def save_verification_details
+          return unless self.class.separate_verification_state_table?
+
+          return unless self.class.available_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
 
         private_class_method :start_verification_batch

ee/spec/models/ee/merge_request_diff_spec.rb

@@ -16,6 +16,56 @@ RSpec.describe MergeRequestDiff do
     stub_external_diffs_setting(enabled: true)
   end
 
+  include_examples 'a replicable model with a separate table for verification state' do
+    let(:verifiable_model_record) { build(:merge_request_diff, :external, external_diff_store: ::ObjectStorage::Store::LOCAL) }
+    let(:unverifiable_model_record) { build(:merge_request_diff) }
+  end
+
+  describe '#after_save' do
+    let(:mr_diff) { build(:merge_request_diff, :external, external_diff_store: ::ObjectStorage::Store::LOCAL) }
+
+    context 'when diff is stored externally and locally' do
+      it 'does not create verification details when diff is without files' do
+        mr_diff[:state] = :without_files
+
+        expect { mr_diff.save! }.not_to change { MergeRequestDiffDetail.count }
+      end
+
+      it 'does not create verification details when diff is empty' do
+        mr_diff[:state] = :empty
+
+        expect { mr_diff.save! }.not_to change { MergeRequestDiffDetail.count }
+      end
+
+      it 'creates verification details' do
+        mr_diff[:state] = :collected
+
+        expect { mr_diff.save! }.to change { MergeRequestDiffDetail.count }.by(1)
+      end
+
+      context 'for a remote stored diff' do
+        before do
+          allow_next_instance_of(MergeRequestDiff) do |mr_diff|
+            allow(mr_diff).to receive(:update_external_diff_store).and_return(true)
+          end
+        end
+
+        it 'does not create verification details' do
+          mr_diff[:state] = :collected
+          mr_diff[:external_diff_store] = ::ObjectStorage::Store::REMOTE
+
+          expect { mr_diff.save! }.not_to change { MergeRequestDiffDetail.count }
+        end
+      end
+    end
+
+    context 'when diff is not stored externally' do
+      it 'does not create verification details' do
+        expect { create(:merge_request_diff, stored_externally: false) }.not_to change { MergeRequestDiffDetail.count }
+      end
+    end
+  end
+
   describe '.with_files_stored_locally' do
     it 'includes states with local storage' do
       create(:merge_request, source_project: project)

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

+# frozen_string_literal: true
+
+# 2 Required let variables that should be valid, unpersisted instances of the same
+# model class. Or valid, persisted instances of the same model class in a not-yet
+# loaded let variable (so we can trigger creation):
+#
+# - verifiable_model_record: should be such that it will be included in the scope
+#                            available_verifiables
+# - unverifiable_model_record: should be such that it will not be included in
+#                              the scope available_verifiables
+
+RSpec.shared_examples 'a replicable model with a separate table for verification state' do
+  include EE::GeoHelpers
+
+  describe '#save_verification_details' do
+    let(:verification_state_table_class) { verifiable_model_record.class.verification_state_table_class }
+
+    context 'when model record is not part of available_verifiables scope' do
+      it 'does not create verification details' do
+        expect { unverifiable_model_record.save! }.not_to change { verification_state_table_class.count }
+      end
+    end
+
+    context 'when model_record is part of available_verifiables scope' do
+      it 'creates verification details' do
+        expect { verifiable_model_record.save! }.to change { verification_state_table_class.count }.by(1)
+      end
+    end
+  end
+end