Revert "Merge branch 'mk/rake-task-verify-remote-files' into 'master'"11-0-stable-prepare-rc14-revert-19501

This reverts commit c79908c138e37f99dee8405e7fad8316be371418.

10 files changed, 41 insertions, 147 deletions

diff --git a/changelogs/unreleased/mk-rake-task-verify-remote-files.yml b/changelogs/unreleased/mk-rake-task-verify-remote-files.yml
deleted file mode 100644
index 772aa11d89b..00000000000
--- a/changelogs/unreleased/mk-rake-task-verify-remote-files.yml
+++ /dev/null
@@ -1,5 +0,0 @@
----
-title: Add support for verifying remote uploads, artifacts, and LFS objects in check rake tasks
-merge_request: 19501
-author:
-type: added
diff --git a/doc/administration/raketasks/check.md b/doc/administration/raketasks/check.md
index 2649bf61d74..7d34d35e7d1 100644
--- a/doc/administration/raketasks/check.md
+++ b/doc/administration/raketasks/check.md
@@ -78,10 +78,9 @@ Example output:
 
 ## Uploaded Files Integrity
 
-Various types of files can be uploaded to a GitLab installation by users.
-These integrity checks can detect missing files. Additionally, for locally
-stored files, checksums are generated and stored in the database upon upload,
-and these checks will verify them against current files.
+Various types of file can be uploaded to a GitLab installation by users.
+Checksums are generated and stored in the database upon upload, and integrity
+checks using those checksums can be run. These checks also detect missing files.
 
 Currently, integrity checks are supported for the following types of file:
 
diff --git a/lib/gitlab/verify/batch_verifier.rb b/lib/gitlab/verify/batch_verifier.rb
index 167ba1b3149..1ef369a4b67 100644
--- a/lib/gitlab/verify/batch_verifier.rb
+++ b/lib/gitlab/verify/batch_verifier.rb
@@ -7,15 +7,13 @@ module Gitlab
         @batch_size = batch_size
         @start = start
         @finish = finish
-
-        fix_google_api_logger
       end
 
       # Yields a Range of IDs and a Hash of failed verifications (object => error)
       def run_batches(&blk)
-        all_relation.in_batches(of: batch_size, start: start, finish: finish) do |batch| # rubocop: disable Cop/InBatches
-          range = batch.first.id..batch.last.id
-          failures = run_batch_for(batch)
+        relation.in_batches(of: batch_size, start: start, finish: finish) do |relation| # rubocop: disable Cop/InBatches
+          range = relation.first.id..relation.last.id
+          failures = run_batch(relation)
 
           yield(range, failures)
         end
@@ -31,56 +29,24 @@ module Gitlab
 
       private
 
-      def run_batch_for(batch)
-        batch.map { |upload| verify(upload) }.compact.to_h
+      def run_batch(relation)
+        relation.map { |upload| verify(upload) }.compact.to_h
       end
 
       def verify(object)
-        local?(object) ? verify_local(object) : verify_remote(object)
-      rescue => err
-        failure(object, err.inspect)
-      end
-
-      def verify_local(object)
         expected = expected_checksum(object)
         actual = actual_checksum(object)
 
-        return failure(object, 'Checksum missing') unless expected.present?
-        return failure(object, 'Checksum mismatch') unless expected == actual
-
-        success
-      end
+        raise 'Checksum missing' unless expected.present?
+        raise 'Checksum mismatch' unless expected == actual
 
-      # We don't calculate checksum for remote objects, so just check existence
-      def verify_remote(object)
-        return failure(object, 'Remote object does not exist') unless remote_object_exists?(object)
-
-        success
-      end
-
-      def success
         nil
-      end
-
-      def failure(object, message)
-        [object, message]
-      end
-
-      # It's already set to Logger::INFO, but acts as if it is set to
-      # Logger::DEBUG, and this fixes it...
-      def fix_google_api_logger
-        if Object.const_defined?('Google::Apis')
-          Google::Apis.logger.level = Logger::INFO
-        end
+      rescue => err
+        [object, err]
       end
 
       # This should return an ActiveRecord::Relation suitable for calling #in_batches on
-      def all_relation
-        raise NotImplementedError.new
-      end
-
-      # Should return true if the object is stored locally
-      def local?(_object)
+      def relation
         raise NotImplementedError.new
       end
 
@@ -93,11 +59,6 @@ module Gitlab
       def actual_checksum(_object)
         raise NotImplementedError.new
       end
-
-      # Be sure to perform a hard check of the remote object (don't just check DB value)
-      def remote_object_exists?(object)
-        raise NotImplementedError.new
-      end
     end
   end
 end
diff --git a/lib/gitlab/verify/job_artifacts.rb b/lib/gitlab/verify/job_artifacts.rb
index dbadfbde9e3..03500a61074 100644
--- a/lib/gitlab/verify/job_artifacts.rb
+++ b/lib/gitlab/verify/job_artifacts.rb
@@ -11,14 +11,10 @@ module Gitlab
 
       private
 
-      def all_relation
+      def relation
         ::Ci::JobArtifact.all
       end
 
-      def local?(artifact)
-        artifact.local_store?
-      end
-
       def expected_checksum(artifact)
         artifact.file_sha256
       end
@@ -26,10 +22,6 @@ module Gitlab
       def actual_checksum(artifact)
         Digest::SHA256.file(artifact.file.path).hexdigest
       end
-
-      def remote_object_exists?(artifact)
-        artifact.file.file.exists?
-      end
     end
   end
 end
diff --git a/lib/gitlab/verify/lfs_objects.rb b/lib/gitlab/verify/lfs_objects.rb
index d3f58a73ac7..970e2a7b718 100644
--- a/lib/gitlab/verify/lfs_objects.rb
+++ b/lib/gitlab/verify/lfs_objects.rb
@@ -11,12 +11,8 @@ module Gitlab
 
       private
 
-      def all_relation
-        LfsObject.all
-      end
-
-      def local?(lfs_object)
-        lfs_object.local_store?
+      def relation
+        LfsObject.with_files_stored_locally
       end
 
       def expected_checksum(lfs_object)
@@ -26,10 +22,6 @@ module Gitlab
       def actual_checksum(lfs_object)
         LfsObject.calculate_oid(lfs_object.file.path)
       end
-
-      def remote_object_exists?(lfs_object)
-        lfs_object.file.file.exists?
-      end
     end
   end
 end
diff --git a/lib/gitlab/verify/rake_task.rb b/lib/gitlab/verify/rake_task.rb
index e190eaddc79..dd138e6b92b 100644
--- a/lib/gitlab/verify/rake_task.rb
+++ b/lib/gitlab/verify/rake_task.rb
@@ -45,7 +45,7 @@ module Gitlab
         return unless verbose?
 
         failures.each do |object, error|
-          say "  - #{verifier.describe(object)}: #{error}".color(:red)
+          say "  - #{verifier.describe(object)}: #{error.inspect}".color(:red)
         end
       end
     end
diff --git a/lib/gitlab/verify/uploads.rb b/lib/gitlab/verify/uploads.rb
index 01f09ab8df7..0ffa71a6d72 100644
--- a/lib/gitlab/verify/uploads.rb
+++ b/lib/gitlab/verify/uploads.rb
@@ -11,12 +11,8 @@ module Gitlab
 
       private
 
-      def all_relation
-        Upload.all
-      end
-
-      def local?(upload)
-        upload.local?
+      def relation
+        Upload.with_files_stored_locally
       end
 
       def expected_checksum(upload)
@@ -26,10 +22,6 @@ module Gitlab
       def actual_checksum(upload)
         Upload.hexdigest(upload.absolute_path)
       end
-
-      def remote_object_exists?(upload)
-        upload.build_uploader.file.exists?
-      end
     end
   end
 end
diff --git a/spec/lib/gitlab/verify/job_artifacts_spec.rb b/spec/lib/gitlab/verify/job_artifacts_spec.rb
index 6e916a56564..ec490bdfde2 100644
--- a/spec/lib/gitlab/verify/job_artifacts_spec.rb
+++ b/spec/lib/gitlab/verify/job_artifacts_spec.rb
@@ -21,38 +21,15 @@ describe Gitlab::Verify::JobArtifacts do
       FileUtils.rm_f(artifact.file.path)
 
       expect(failures.keys).to contain_exactly(artifact)
-      expect(failure).to include('No such file or directory')
-      expect(failure).to include(artifact.file.path)
+      expect(failure).to be_a(Errno::ENOENT)
+      expect(failure.to_s).to include(artifact.file.path)
     end
 
     it 'fails artifacts with a mismatched checksum' do
       File.truncate(artifact.file.path, 0)
 
       expect(failures.keys).to contain_exactly(artifact)
-      expect(failure).to include('Checksum mismatch')
-    end
-
-    context 'with remote files' do
-      let(:file) { double(:file) }
-
-      before do
-        stub_artifacts_object_storage
-        artifact.update!(file_store: ObjectStorage::Store::REMOTE)
-        expect(CarrierWave::Storage::Fog::File).to receive(:new).and_return(file)
-      end
-
-      it 'passes artifacts in object storage that exist' do
-        expect(file).to receive(:exists?).and_return(true)
-
-        expect(failures).to eq({})
-      end
-
-      it 'fails artifacts in object storage that do not exist' do
-        expect(file).to receive(:exists?).and_return(false)
-
-        expect(failures.keys).to contain_exactly(artifact)
-        expect(failure).to include('Remote object does not exist')
-      end
+      expect(failure.to_s).to include('Checksum mismatch')
     end
   end
 end
diff --git a/spec/lib/gitlab/verify/lfs_objects_spec.rb b/spec/lib/gitlab/verify/lfs_objects_spec.rb
index 2feaedd6f14..0f890e2c7ce 100644
--- a/spec/lib/gitlab/verify/lfs_objects_spec.rb
+++ b/spec/lib/gitlab/verify/lfs_objects_spec.rb
@@ -21,37 +21,30 @@ describe Gitlab::Verify::LfsObjects do
       FileUtils.rm_f(lfs_object.file.path)
 
       expect(failures.keys).to contain_exactly(lfs_object)
-      expect(failure).to include('No such file or directory')
-      expect(failure).to include(lfs_object.file.path)
+      expect(failure).to be_a(Errno::ENOENT)
+      expect(failure.to_s).to include(lfs_object.file.path)
     end
 
     it 'fails LFS objects with a mismatched oid' do
       File.truncate(lfs_object.file.path, 0)
 
       expect(failures.keys).to contain_exactly(lfs_object)
-      expect(failure).to include('Checksum mismatch')
+      expect(failure.to_s).to include('Checksum mismatch')
     end
 
     context 'with remote files' do
-      let(:file) { double(:file) }
-
       before do
         stub_lfs_object_storage
-        lfs_object.update!(file_store: ObjectStorage::Store::REMOTE)
-        expect(CarrierWave::Storage::Fog::File).to receive(:new).and_return(file)
       end
 
-      it 'passes LFS objects in object storage that exist' do
-        expect(file).to receive(:exists?).and_return(true)
-
-        expect(failures).to eq({})
-      end
+      it 'skips LFS objects in object storage' do
+        local_failure = create(:lfs_object)
+        create(:lfs_object, :object_storage)
 
-      it 'fails LFS objects in object storage that do not exist' do
-        expect(file).to receive(:exists?).and_return(false)
+        failures = {}
+        described_class.new(batch_size: 10).run_batches { |_, failed| failures.merge!(failed) }
 
-        expect(failures.keys).to contain_exactly(lfs_object)
-        expect(failure).to include('Remote object does not exist')
+        expect(failures.keys).to contain_exactly(local_failure)
       end
     end
   end
diff --git a/spec/lib/gitlab/verify/uploads_spec.rb b/spec/lib/gitlab/verify/uploads_spec.rb
index 296866d3319..85768308edc 100644
--- a/spec/lib/gitlab/verify/uploads_spec.rb
+++ b/spec/lib/gitlab/verify/uploads_spec.rb
@@ -23,44 +23,37 @@ describe Gitlab::Verify::Uploads do
       FileUtils.rm_f(upload.absolute_path)
 
       expect(failures.keys).to contain_exactly(upload)
-      expect(failure).to include('No such file or directory')
-      expect(failure).to include(upload.absolute_path)
+      expect(failure).to be_a(Errno::ENOENT)
+      expect(failure.to_s).to include(upload.absolute_path)
     end
 
     it 'fails uploads with a mismatched checksum' do
       upload.update!(checksum: 'something incorrect')
 
       expect(failures.keys).to contain_exactly(upload)
-      expect(failure).to include('Checksum mismatch')
+      expect(failure.to_s).to include('Checksum mismatch')
     end
 
     it 'fails uploads with a missing precalculated checksum' do
       upload.update!(checksum: '')
 
       expect(failures.keys).to contain_exactly(upload)
-      expect(failure).to include('Checksum missing')
+      expect(failure.to_s).to include('Checksum missing')
     end
 
     context 'with remote files' do
-      let(:file) { double(:file) }
-
       before do
         stub_uploads_object_storage(AvatarUploader)
-        upload.update!(store: ObjectStorage::Store::REMOTE)
-        expect(CarrierWave::Storage::Fog::File).to receive(:new).and_return(file)
       end
 
-      it 'passes uploads in object storage that exist' do
-        expect(file).to receive(:exists?).and_return(true)
-
-        expect(failures).to eq({})
-      end
+      it 'skips uploads in object storage' do
+        local_failure = create(:upload)
+        create(:upload, :object_storage)
 
-      it 'fails uploads in object storage that do not exist' do
-        expect(file).to receive(:exists?).and_return(false)
+        failures = {}
+        described_class.new(batch_size: 10).run_batches { |_, failed| failures.merge!(failed) }
 
-        expect(failures.keys).to contain_exactly(upload)
-        expect(failure).to include('Remote object does not exist')
+        expect(failures.keys).to contain_exactly(local_failure)
       end
     end
   end