Merge branch 'mk/remove-local-method-requirement' into 'master'

Geo: Minor refactors to verification

See merge request gitlab-org/gitlab!53340

app/models/packages/package_file.rb

@@ -55,10 +55,6 @@ class Packages::PackageFile < ApplicationRecord
     Gitlab::Routing.url_helpers.download_project_package_file_path(project, self)
   end
 
-  def local?
-    file_store == ::Packages::PackageFileUploader::Store::LOCAL
-  end
-
   private
 
   def update_size_from_file

app/models/terraform/state_version.rb

@@ -17,10 +17,6 @@ module Terraform
     mount_file_store_uploader StateUploader
 
     delegate :project_id, :uuid, to: :terraform_state, allow_nil: true
-
-    def local?
-      file_store == ObjectStorage::Store::LOCAL
-    end
   end
 end
 

doc/development/geo/framework.md

@@ -189,11 +189,6 @@ For example, to add support for files referenced by a `Widget` model with a
 
      mount_uploader :file, WidgetUploader
 
-     def local?
-       # Must to be implemented, Check the uploader's storage types
-       file_store == ObjectStorage::Store::LOCAL
-     end
-
      # @param primary_key_in [Range, Widget] arg to pass to primary_key_in scope
      # @return [ActiveRecord::Relation<Widget>] everything that should be synced to this node, restricted by primary key
      def self.replicables_for_current_secondary(primary_key_in)

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

@@ -56,6 +56,26 @@ module Geo
       ).execute
     end
 
+    # Returns a checksum of the file
+    #
+    # @return [String] SHA256 hash of the carrierwave file
+    def calculate_checksum
+      raise 'File is not checksummable' unless checksummable?
+
+      model.hexdigest(carrierwave_uploader.path)
+    end
+
+    # Returns whether the file exists on disk or in remote storage
+    #
+    # Does a hard check because we are doing these checks for replication or
+    # verification purposes, so we should not just trust the data in the DB if
+    # we don't absolutely have to.
+    #
+    # @return [Boolean] whether the file exists on disk or in remote storage
+    def file_exists?
+      carrierwave_uploader.file.exists?
+    end
+
     private
 
     def download
@@ -65,5 +85,12 @@ module Geo
     def deleted_params
       { model_record_id: model_record.id, blob_path: blob_path }
     end
+
+    # Return whether it's capable of generating a checksum of itself
+    #
+    # @return [Boolean] whether it can generate a checksum
+    def checksummable?
+      carrierwave_uploader.file_storage? && file_exists?
+    end
   end
 end

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

@@ -118,8 +118,9 @@ module Geo
       end
     end
 
+    # Schedules a verification job after a model record is created/updated
     def after_verifiable_update
-      verify_async if needs_checksum?
+      verify_async if should_primary_verify?
     end
 
     def verify_async
@@ -137,7 +138,7 @@ module Geo
     # state.
     def verify
       verification_state_tracker.track_checksum_attempt! do
-        model_record.calculate_checksum
+        calculate_checksum
       end
     end
 
@@ -146,14 +147,7 @@ module Geo
     # @param [String] checksum
     # @return [Boolean] whether checksum matches
     def matches_checksum?(checksum)
-      model_record.verification_checksum == checksum
-    end
-
-    def needs_checksum?
-      return false unless self.class.verification_enabled?
-      return true unless model_record.respond_to?(:needs_checksum?)
-
-      model_record.needs_checksum?
+      primary_checksum == checksum
     end
 
     # Checksum value from the main database
@@ -170,5 +164,25 @@ module Geo
     def verification_state_tracker
       Gitlab::Geo.secondary? ? registry : model_record
     end
+
+    # @abstract
+    # @return [String] a checksum representing the data
+    def calculate_checksum
+      raise NotImplementedError, "#{self.class} does not implement #{__method__}"
+    end
+
+    private
+
+    def should_primary_verify?
+      self.class.verification_enabled? &&
+       primary_checksum.nil? && # Some models may populate this as part of creating the record
+       checksummable?
+    end
+
+    # @abstract
+    # @return [Boolean] whether the replicable is capable of checksumming itself
+    def checksummable?
+      raise NotImplementedError, "#{self.class} does not implement #{__method__}"
+    end
   end
 end

ee/app/models/ee/merge_request_diff.rb

@@ -59,10 +59,6 @@ module EE
       super || build_merge_request_diff_detail
     end
 
-    def local?
-      stored_externally? && external_diff_store == ExternalDiffUploader::Store::LOCAL
-    end
-
     def log_geo_deleted_event
       # Keep empty for now. Should be addressed in future
       # by https://gitlab.com/gitlab-org/gitlab/issues/33817

ee/app/replicators/geo/merge_request_diff_replicator.rb

@@ -3,6 +3,7 @@
 module Geo
   class MergeRequestDiffReplicator < Gitlab::Geo::Replicator
     include ::Geo::BlobReplicatorStrategy
+    extend ::Gitlab::Utils::Override
 
     def self.model
       ::MergeRequestDiff
@@ -16,8 +17,12 @@ module Geo
       model_record.external_diff
     end
 
-    def needs_checksum?
-      false
+    private
+
+    # Only external diffs can be checksummed
+    override :checksummable?
+    def checksummable?
+      model_record.stored_externally? && super
     end
   end
 end

ee/app/replicators/geo/snippet_repository_replicator.rb

@@ -12,10 +12,6 @@ module Geo
       ::Gitlab::GitAccessSnippet
     end
 
-    def needs_checksum?
-      false
-    end
-
     def repository
       model_record.repository
     end

ee/app/replicators/geo/terraform_state_version_replicator.rb

@@ -11,9 +11,5 @@ module Geo
     def self.model
       ::Terraform::StateVersion
     end
-
-    def needs_checksum?
-      false
-    end
   end
 end

ee/lib/gitlab/geo/replicable_model.rb

@@ -48,44 +48,6 @@ module Gitlab
         raise NotImplementedError, 'There is no Replicator defined for this model'
       end
 
-      # Returns a checksum of the file (assumed to be a "blob" type)
-      #
-      # @return [String] SHA256 hash of the carrierwave file
-      def calculate_checksum
-        return unless checksummable?
-
-        self.class.hexdigest(replicator.carrierwave_uploader.path)
-      end
-
-      # Checks whether model needs checksum to be performed
-      #
-      # Conditions:
-      # - No checksum is present
-      # - It's capable of generating a checksum of itself
-      #
-      # @return [Boolean]
-      def needs_checksum?
-        verification_checksum.nil? && checksummable?
-      end
-
-      # Return whether its capable of generating a checksum of itself
-      #
-      # @return [Boolean] whether it can generate a checksum
-      def checksummable?
-        local? && file_exist?
-      end
-
-      # This checks for existence of the file on storage
-      #
-      # @return [Boolean] whether the file exists on storage
-      def file_exist?
-        if local?
-          File.exist?(replicator.carrierwave_uploader.path)
-        else
-          replicator.carrierwave_uploader.exists?
-        end
-      end
-
       def in_replicables_for_current_secondary?
         self.class.replicables_for_current_secondary(self).exists?
       end

ee/lib/gitlab/geo/replication/blob_retriever.rb

@@ -19,7 +19,7 @@ module Gitlab
 
         def execute
           return error("#{replicator.replicable_name} not found") unless recorded_file
-          return file_not_found(recorded_file) unless recorded_file.file_exist?
+          return file_not_found(recorded_file) unless replicator.file_exists?
           return error('Checksum mismatch') unless matches_checksum?
 
           success(replicator.carrierwave_uploader)

ee/spec/models/packages/package_file_spec.rb

@@ -4,22 +4,6 @@ require 'spec_helper'
 RSpec.describe Packages::PackageFile, type: :model do
   include ::EE::GeoHelpers
 
-  describe '#calculate_checksum' do
-    let(:package_file) { create(:conan_package_file, :conan_recipe_file) }
-
-    it 'returns SHA256 sum of the file' do
-      expected = Digest::SHA256.file(package_file.file.path).hexdigest
-
-      expect(package_file.calculate_checksum).to eq(expected)
-    end
-
-    it 'returns nil for a non-existent file' do
-      allow(package_file).to receive(:file_exist?).and_return(false)
-
-      expect(package_file.calculate_checksum).to eq(nil)
-    end
-  end
-
   context 'new file' do
     it 'calls checksum worker' do
       stub_primary_node

ee/spec/services/geo/event_service_spec.rb

@@ -23,7 +23,7 @@ RSpec.describe Geo::EventService do
     it 'executes the consume part of the replication' do
       subject.execute
 
-      expect(model_record.file_exist?).to be_truthy
+      expect(model_record.file.exists?).to be_truthy
     end
   end
 end

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

@@ -169,4 +169,32 @@ RSpec.shared_examples 'a blob replicator' do
       end
     end
   end
+
+  describe '#calculate_checksum' do
+    context 'when the file is locally stored' do
+      context 'when the file exists' do
+        it 'returns hexdigest of the file' do
+          expected = described_class.model.hexdigest(subject.carrierwave_uploader.path)
+
+          expect(subject.calculate_checksum).to eq(expected)
+        end
+      end
+
+      context 'when the file does not exist' do
+        it 'raises an error' do
+          allow(subject).to receive(:file_exists?).and_return(false)
+
+          expect { subject.calculate_checksum }.to raise_error('File is not checksummable')
+        end
+      end
+    end
+
+    context 'when the file is remotely stored' do
+      it 'raises an error' do
+        allow(subject.carrierwave_uploader).to receive(:file_storage?).and_return(false)
+
+        expect { subject.calculate_checksum }.to raise_error('File is not checksummable')
+      end
+    end
+  end
 end

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

@@ -299,8 +299,11 @@ RSpec.shared_examples 'a verifiable replicator' do
 
   describe '#after_verifiable_update' do
     it 'calls verify_async if needed' do
+      allow(described_class).to receive(:verification_enabled?).and_return(true)
+      allow(replicator).to receive(:primary_checksum).and_return(nil)
+      allow(replicator).to receive(:checksummable?).and_return(true)
+
       expect(replicator).to receive(:verify_async)
-      expect(replicator).to receive(:needs_checksum?).and_return(true)
 
       replicator.after_verifiable_update
     end
@@ -329,8 +332,8 @@ RSpec.shared_examples 'a verifiable replicator' do
     it 'wraps the checksum calculation in track_checksum_attempt!' do
       tracker = double('tracker')
       allow(replicator).to receive(:verification_state_tracker).and_return(tracker)
+      allow(replicator).to receive(:calculate_checksum).and_return('abc123')
 
-      expect(model_record).to receive(:calculate_checksum)
       expect(tracker).to receive(:track_checksum_attempt!).and_yield
 
       replicator.verify