Merge branch 'sh-geo-fix-snippet-downloads' into 'master'

Fix personal snippet attachments not downloading in Geo secondaries

Closes #3644

See merge request gitlab-org/gitlab-ee!3091

app/services/geo/file_service.rb

@@ -2,7 +2,7 @@ module Geo
   class FileService
     attr_reader :object_type, :object_db_id
 
-    DEFAULT_OBJECT_TYPES = %w[attachment avatar file].freeze
+    DEFAULT_OBJECT_TYPES = %w[attachment avatar file personal_file].freeze
     DEFAULT_SERVICE_TYPE = 'file'.freeze
 
     def initialize(object_type, object_db_id)

app/workers/geo/file_download_dispatch_worker.rb

@@ -16,7 +16,7 @@ module Geo
     end
 
     def find_object_ids
-      downloaded_ids = find_downloaded_ids([:attachment, :avatar, :file])
+      downloaded_ids = find_downloaded_ids(Geo::FileService::DEFAULT_OBJECT_TYPES)
 
       unsynched_downloads = filter_downloaded_ids(
         current_node.uploads, downloaded_ids, Upload.table_name)
@@ -25,7 +25,7 @@ module Geo
         .order(created_at: :desc)
         .limit(db_retrieve_batch_size)
         .pluck(:id, :uploader)
-        .map { |id, uploader| [id, uploader.sub(/Uploader\z/, '').downcase] }
+        .map { |id, uploader| [id, uploader.sub(/Uploader\z/, '').underscore] }
     end
 
     def find_lfs_object_ids

changelogs/unreleased-ee/sh-geo-fix-snippet-downloads.yml

+---
+title: Fix personal snippets not downloading in Geo secondaries
+merge_request:
+author:
+type: fixed

spec/factories/uploads.rb

@@ -4,5 +4,10 @@ FactoryGirl.define do
     path { "uploads/-/system/project/avatar/avatar.jpg" }
     size 100.kilobytes
     uploader "AvatarUploader"
+
+    trait :personal_snippet do
+      model { build(:personal_snippet) }
+      uploader "PersonalFileUploader"
+    end
   end
 end

spec/services/geo/file_download_service_spec.rb

@@ -75,6 +75,21 @@ describe Geo::FileDownloadService do
       end
     end
 
+    context 'with a snippet' do
+      let(:upload) { create(:upload, :personal_snippet) }
+
+      subject { described_class.new(:personal_file, upload.id) }
+
+      it 'downloads the file' do
+        allow_any_instance_of(Gitlab::ExclusiveLease)
+          .to receive(:try_obtain).and_return(true)
+        allow_any_instance_of(Gitlab::Geo::FileTransfer)
+          .to receive(:download_from_primary).and_return(100)
+
+        expect { subject.execute }.to change { Geo::FileRegistry.count }.by(1)
+      end
+    end
+
     context 'with file upload' do
       let(:project) { create(:project) }
       let(:upload) { Upload.find_by(model: project, uploader: 'FileUploader') }

spec/workers/geo/file_download_dispatch_worker_spec.rb

@@ -57,7 +57,7 @@ describe Geo::FileDownloadDispatchWorker, :postgresql do
 
     # Test the case where we have:
     #
-    # 1. A total of 8 files in the queue, and we can load a maximimum of 5 and send 2 at a time.
+    # 1. A total of 10 files in the queue, and we can load a maximimum of 5 and send 2 at a time.
     # 2. We send 2, wait for 1 to finish, and then send again.
     it 'attempts to load a new batch without pending downloads' do
       stub_const('Geo::BaseSchedulerWorker::DB_RETRIEVE_BATCH_SIZE', 5)
@@ -67,15 +67,17 @@ describe Geo::FileDownloadDispatchWorker, :postgresql do
       create_list(:lfs_object, 2, :with_file)
       create_list(:user, 2, avatar: avatar)
       create_list(:note, 2, :with_attachment)
+      create_list(:upload, 2, :personal_snippet)
       create(:appearance, logo: avatar, header_logo: avatar)
 
-      expect(GeoFileDownloadWorker).to receive(:perform_async).exactly(8).times.and_call_original
-      # For 8 downloads, we expect three database reloads:
+      expect(GeoFileDownloadWorker).to receive(:perform_async).exactly(10).times.and_call_original
+      # For 10 downloads, we expect four database reloads:
       # 1. Load the first batch of 5.
-      # 2. 4 get sent out, 1 remains. This triggers another reload, which loads in the remaining 4.
+      # 2. 4 get sent out, 1 remains. This triggers another reload, which loads in the next 5.
+      # 3. Those 4 get sent out, and 1 remains.
       # 3. Since the second reload filled the pipe with 4, we need to do a final reload to ensure
       #    zero are left.
-      expect(subject).to receive(:load_pending_resources).exactly(3).times.and_call_original
+      expect(subject).to receive(:load_pending_resources).exactly(4).times.and_call_original
 
       Sidekiq::Testing.inline! do
         subject.perform