Merge branch '6541-custom-favicons-not-being-replicated-by-geo' into 'master'

Resolve "Custom favicons not being replicated by Geo"

Closes #6541

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

ee/app/services/geo/file_service.rb

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

ee/changelogs/unreleased/6541-custom-favicons-not-being-replicated-by-geo.yml

+---
+title: "[Geo] Fix: Custom favicons not being replicated by Geo"
+merge_request: 6860
+author:
+type: fixed

ee/spec/factories/geo/file_registry.rb

@@ -10,6 +10,7 @@ FactoryBot.define do
     trait(:lfs) { file_type :lfs }
     trait(:namespace_file) { file_type :namespace_file }
     trait(:personal_file) { file_type :personal_file }
+    trait(:favicon) { file_type :favicon }
     trait(:import_export) { file_type :import_export }
 
     trait :with_file do

ee/spec/lib/gitlab/geo/file_uploader_spec.rb

@@ -106,6 +106,12 @@ describe Gitlab::Geo::FileUploader, :geo do
       end
     end
 
+    context 'with favicon upload' do
+      it_behaves_like "returns necessary params for sending a file from an API endpoint" do
+        let(:upload) { create(:upload, :favicon_upload) }
+      end
+    end
+
     context 'with namespace file upload' do
       it_behaves_like "returns necessary params for sending a file from an API endpoint" do
         let(:upload) { create(:upload, :namespace_upload) }

ee/spec/services/geo/file_download_service_spec.rb

@@ -279,6 +279,12 @@ describe Geo::FileDownloadService do
       end
     end
 
+    context 'with a favicon' do
+      it_behaves_like "a service that downloads the file and registers the sync result", 'favicon' do
+        let(:file) { create(:upload, :favicon_upload) }
+      end
+    end
+
     context 'with a snippet' do
       it_behaves_like "a service that downloads the file and registers the sync result", 'personal_file' do
         let(:file) { create(:upload, :personal_snippet_upload) }

ee/spec/services/geo/file_registry_removal_service_spec.rb

@@ -190,5 +190,27 @@ describe Geo::FileRegistryRemovalService do
         it_behaves_like 'removes'
       end
     end
+
+    context 'with favicon' do
+      let(:appearance) { create(:appearance) }
+      let(:file) { fixture_file_upload('spec/fixtures/dk.png', 'image/png') }
+      let!(:upload) do
+        FaviconUploader.new(appearance).store!(file)
+        Upload.find_by(model: appearance, uploader: FaviconUploader)
+      end
+      let!(:file_registry) { create(:geo_file_registry, :favicon, file_id: upload.id) }
+      let!(:file_path) { upload.build_uploader.file.path }
+
+      it_behaves_like 'removes'
+
+      context 'migrated to object storage' do
+        before do
+          stub_uploads_object_storage(FaviconUploader)
+          upload.update_column(:store, PersonalFileUploader::Store::REMOTE)
+        end
+
+        it_behaves_like 'removes'
+      end
+    end
   end
 end

ee/spec/workers/geo/migrated_local_files_clean_up_worker_spec.rb

@@ -59,6 +59,7 @@ describe Geo::MigratedLocalFilesCleanUpWorker, :geo do
       let(:issuable_upload) { create(:upload, :issuable_upload) }
       let(:namespace_upload) { create(:upload, :namespace_upload) }
       let(:attachment_upload) { create(:upload, :attachment_upload) }
+      let(:favicon_upload) { create(:upload, :favicon_upload) }
 
       before do
         create(:geo_file_registry, :avatar, file_id: avatar_upload.id)
@@ -66,6 +67,7 @@ describe Geo::MigratedLocalFilesCleanUpWorker, :geo do
         create(:geo_file_registry, :file, file_id: issuable_upload.id)
         create(:geo_file_registry, :namespace_file, file_id: namespace_upload.id)
         create(:geo_file_registry, :attachment, file_id: attachment_upload.id)
+        create(:geo_file_registry, :favicon, file_id: favicon_upload.id)
       end
 
       it 'schedules nothing for attachments stored locally' do
@@ -74,6 +76,7 @@ describe Geo::MigratedLocalFilesCleanUpWorker, :geo do
         expect(worker).not_to receive(:schedule_job).with(anything, issuable_upload.id)
         expect(worker).not_to receive(:schedule_job).with(anything, namespace_upload.id)
         expect(worker).not_to receive(:schedule_job).with(anything, attachment_upload.id)
+        expect(worker).not_to receive(:schedule_job).with(anything, favicon_upload.id)
 
         worker.perform
       end
@@ -85,12 +88,14 @@ describe Geo::MigratedLocalFilesCleanUpWorker, :geo do
           stub_uploads_object_storage(FileUploader)
           stub_uploads_object_storage(NamespaceFileUploader)
           stub_uploads_object_storage(AttachmentUploader)
+          stub_uploads_object_storage(FaviconUploader)
 
           avatar_upload.update_column(:store, FileUploader::Store::REMOTE)
           personal_snippet_upload.update_column(:store, FileUploader::Store::REMOTE)
           issuable_upload.update_column(:store, FileUploader::Store::REMOTE)
           namespace_upload.update_column(:store, FileUploader::Store::REMOTE)
           attachment_upload.update_column(:store, FileUploader::Store::REMOTE)
+          favicon_upload.update_column(:store, FileUploader::Store::REMOTE)
         end
 
         it 'schedules jobs for uploads stored remotely and synced locally' do
@@ -99,6 +104,7 @@ describe Geo::MigratedLocalFilesCleanUpWorker, :geo do
           expect(worker).to receive(:schedule_job).with('file', issuable_upload.id)
           expect(worker).to receive(:schedule_job).with('namespace_file', namespace_upload.id)
           expect(worker).to receive(:schedule_job).with('attachment', attachment_upload.id)
+          expect(worker).to receive(:schedule_job).with('favicon', favicon_upload.id)
 
           worker.perform
         end
@@ -109,6 +115,7 @@ describe Geo::MigratedLocalFilesCleanUpWorker, :geo do
           expect(Geo::FileRegistryRemovalWorker).to receive(:perform_async).with('file', issuable_upload.id)
           expect(Geo::FileRegistryRemovalWorker).to receive(:perform_async).with('namespace_file', namespace_upload.id)
           expect(Geo::FileRegistryRemovalWorker).to receive(:perform_async).with('attachment', attachment_upload.id)
+          expect(Geo::FileRegistryRemovalWorker).to receive(:perform_async).with('favicon', favicon_upload.id)
 
           worker.perform
         end

spec/factories/uploads.rb

@@ -46,6 +46,13 @@ FactoryBot.define do
       secret SecureRandom.hex
     end
 
+    trait :favicon_upload do
+      model { build(:appearance) }
+      path { File.join(secret, filename) }
+      uploader "FaviconUploader"
+      secret SecureRandom.hex
+    end
+
     trait :attachment_upload do
       transient do
         mount_point :attachment

spec/lib/gitlab/cleanup/project_uploads_spec.rb

@@ -244,9 +244,11 @@ describe Gitlab::Cleanup::ProjectUploads do
         orphaned1 = create(:upload, :personal_snippet_upload, :with_file)
         orphaned2 = create(:upload, :namespace_upload, :with_file)
         orphaned3 = create(:upload, :attachment_upload, :with_file)
+        orphaned4 = create(:upload, :favicon_upload, :with_file)
         paths << orphaned1.absolute_path
         paths << orphaned2.absolute_path
         paths << orphaned3.absolute_path
+        paths << orphaned4.absolute_path
         Upload.delete_all
 
         expect(logger).not_to receive(:info).with(/move|fix/i)