Merge branch 'ag-remove-autosave' into 'master'

Save verification details conditionally after saving a replicable

See merge request gitlab-org/gitlab!70615

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