Rename `build_uploader to `retrieve_uploader``

This change is to clarify that we are not only rebuilding the uploader
but also fetching and caching the file locally, which is an expensive
operation.

app/controllers/concerns/uploads_actions.rb

@@ -91,7 +91,7 @@ module UploadsActions
 
     upload_paths = uploader.upload_paths(params[:filename])
     upload = Upload.find_by(model: model, uploader: uploader_class.to_s, path: upload_paths)
-    upload&.build_uploader
+    upload&.retrieve_uploader
   end
   # rubocop: enable CodeReuse/ActiveRecord
 

app/models/upload.rb

@@ -58,9 +58,23 @@ class Upload < ApplicationRecord
     self.checksum = Digest::SHA256.file(absolute_path).hexdigest
   end
 
+  # Initialize the associated Uploader class with current model
+  #
+  # @param [String] mounted_as
+  # @return [GitlabUploader] one of the subclasses, defined at the model's uploader attribute
   def build_uploader(mounted_as = nil)
     uploader_class.new(model, mounted_as || mount_point).tap do |uploader|
       uploader.upload = self
+    end
+  end
+
+  # Initialize the associated Uploader class with current model and
+  # retrieve existing file from the store to a local cache
+  #
+  # @param [String] mounted_as
+  # @return [GitlabUploader] one of the subclasses, defined at the model's uploader attribute
+  def retrieve_uploader(mounted_as = nil)
+    build_uploader(mounted_as).tap do |uploader|
       uploader.retrieve_from_store!(identifier)
     end
   end
@@ -72,7 +86,7 @@ class Upload < ApplicationRecord
     exist = if local?
               File.exist?(absolute_path)
             else
-              build_uploader.exists?
+              retrieve_uploader.exists?
             end
 
     # Help sysadmins find missing upload files
@@ -111,7 +125,7 @@ class Upload < ApplicationRecord
   private
 
   def delete_file!
-    build_uploader.remove!
+    retrieve_uploader.remove!
   end
 
   def foreground_checksummable?

app/workers/import_issues_csv_worker.rb

@@ -12,7 +12,7 @@ class ImportIssuesCsvWorker
     @project = Project.find(project_id)
     @upload = Upload.find(upload_id)
 
-    importer = Issues::ImportCsvService.new(@user, @project, @upload.build_uploader)
+    importer = Issues::ImportCsvService.new(@user, @project, @upload.retrieve_uploader)
     importer.execute
 
     @upload.destroy

app/workers/object_storage/background_move_worker.rb

@@ -22,7 +22,7 @@ module ObjectStorage
 
     def build_uploader(subject, mount_point)
       case subject
-      when Upload then subject.build_uploader(mount_point)
+      when Upload then subject.retrieve_uploader(mount_point)
       else
         subject.send(mount_point) # rubocop:disable GitlabSecurity/PublicSend
       end

app/workers/object_storage/migrate_uploads_worker.rb

@@ -119,7 +119,7 @@ module ObjectStorage
     end
 
     def build_uploaders(uploads)
-      uploads.map { |upload| upload.build_uploader(@mounted_as) }
+      uploads.map { |upload| upload.retrieve_uploader(@mounted_as) }
     end
 
     def migrate(uploads)

ee/app/services/geo/file_registry_removal_service.rb

@@ -79,7 +79,7 @@ module Geo
         when :job_artifact
           Ci::JobArtifact.find(object_db_id).file
         when *Gitlab::Geo::Replication::USER_UPLOADS_OBJECT_TYPES
-          Upload.find(object_db_id).build_uploader
+          Upload.find(object_db_id).retrieve_uploader
         else
           raise NameError, "Unrecognized type: #{object_type}"
         end

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

@@ -13,7 +13,7 @@ module Gitlab
           return file_not_found(recorded_file) unless recorded_file.exist?
           return error('Upload not found') unless valid?
 
-          success(recorded_file.build_uploader)
+          success(recorded_file.retrieve_uploader)
         end
 
         private

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

@@ -30,7 +30,7 @@ module Gitlab
             file_type: file_type,
             file_id: upload.id,
             filename: upload.absolute_path,
-            uploader: upload.build_uploader,
+            uploader: upload.retrieve_uploader,
             expected_checksum: upload.checksum,
             request_data: build_request_data(file_type, upload)
           }
@@ -40,7 +40,7 @@ module Gitlab
           {
             file_type: file_type,
             file_id: upload.id,
-            uploader: upload.build_uploader,
+            uploader: upload.retrieve_uploader,
             request_data: build_request_data(file_type, upload)
           }
         end

ee/spec/lib/gitlab/geo/replication/file_downloader_spec.rb

@@ -43,12 +43,12 @@ describe Gitlab::Geo::Replication::FileDownloader, :geo do
   def stub_geo_file_transfer(file_type, upload)
     url = primary_node.geo_transfers_url(file_type, upload.id.to_s)
 
-    stub_request(:get, url).to_return(status: 200, body: upload.build_uploader.file.read, headers: {})
+    stub_request(:get, url).to_return(status: 200, body: upload.retrieve_uploader.file.read, headers: {})
   end
 
   def stub_geo_file_transfer_object_storage(file_type, upload)
     url = primary_node.geo_transfers_url(file_type, upload.id.to_s)
 
-    stub_request(:get, url).to_return(status: 307, body: upload.build_uploader.url, headers: {})
+    stub_request(:get, url).to_return(status: 307, body: upload.retrieve_uploader.url, headers: {})
   end
 end

ee/spec/lib/gitlab/geo/replication/file_transfer_spec.rb

@@ -44,7 +44,7 @@ describe Gitlab::Geo::Replication::FileTransfer do
 
     context 'when the HTTP response is successful' do
       it 'returns a successful result' do
-        content = upload.build_uploader.file.read
+        content = upload.retrieve_uploader.file.read
         size = content.bytesize
         stub_request(:get, subject.resource_url).to_return(status: 200, body: content)
 

ee/spec/services/geo/file_registry_removal_service_spec.rb

@@ -120,7 +120,7 @@ describe Geo::FileRegistryRemovalService do
     context 'with avatar' do
       let!(:upload) { create(:user, :with_avatar).avatar.upload }
       let!(:file_registry) { create(:geo_file_registry, :avatar, file_id: upload.id) }
-      let!(:file_path) { upload.build_uploader.file.path }
+      let!(:file_path) { upload.retrieve_uploader.file.path }
 
       it_behaves_like 'removes'
 
@@ -137,7 +137,7 @@ describe Geo::FileRegistryRemovalService do
     context 'with attachment' do
       let!(:upload) { create(:note, :with_attachment).attachment.upload }
       let!(:file_registry) { create(:geo_file_registry, :attachment, file_id: upload.id) }
-      let!(:file_path) { upload.build_uploader.file.path }
+      let!(:file_path) { upload.retrieve_uploader.file.path }
 
       it_behaves_like 'removes'
 
@@ -154,7 +154,7 @@ describe Geo::FileRegistryRemovalService do
     context 'with file' do
       let!(:upload) { create(:user, :with_avatar).avatar.upload }
       let!(:file_registry) { create(:geo_file_registry, :avatar, file_id: upload.id) }
-      let!(:file_path) { upload.build_uploader.file.path }
+      let!(:file_path) { upload.retrieve_uploader.file.path }
 
       it_behaves_like 'removes'
 
@@ -177,7 +177,7 @@ describe Geo::FileRegistryRemovalService do
       end
 
       let!(:file_registry) { create(:geo_file_registry, :namespace_file, file_id: upload.id) }
-      let!(:file_path) { upload.build_uploader.file.path }
+      let!(:file_path) { upload.retrieve_uploader.file.path }
 
       it_behaves_like 'removes'
 
@@ -199,7 +199,7 @@ describe Geo::FileRegistryRemovalService do
         Upload.find_by(model: snippet, uploader: PersonalFileUploader.name)
       end
       let!(:file_registry) { create(:geo_file_registry, :personal_file, file_id: upload.id) }
-      let!(:file_path) { upload.build_uploader.file.path }
+      let!(:file_path) { upload.retrieve_uploader.file.path }
 
       it_behaves_like 'removes'
 
@@ -221,7 +221,7 @@ describe Geo::FileRegistryRemovalService do
         Upload.find_by(model: appearance, uploader: FaviconUploader.name)
       end
       let!(:file_registry) { create(:geo_file_registry, :favicon, file_id: upload.id) }
-      let!(:file_path) { upload.build_uploader.file.path }
+      let!(:file_path) { upload.retrieve_uploader.file.path }
 
       it_behaves_like 'removes'
 

lib/gitlab/background_migration/legacy_upload_mover.rb

@@ -92,7 +92,7 @@ module Gitlab
 
       def legacy_file_uploader
         strong_memoize(:legacy_file_uploader) do
-          uploader = upload.build_uploader
+          uploader = upload.retrieve_uploader
           uploader.retrieve_from_store!(File.basename(upload.path))
           uploader
         end

lib/gitlab/import_export/uploads_manager.rb

@@ -68,7 +68,7 @@ module Gitlab
           yield(@project.avatar)
         else
           project_uploads_except_avatar(avatar_path).find_each(batch_size: UPLOADS_BATCH_SIZE) do |upload|
-            yield(upload.build_uploader)
+            yield(upload.retrieve_uploader)
           end
         end
       end

lib/gitlab/sanitizers/exif.rb

@@ -68,7 +68,7 @@ module Gitlab
         }
 
         relation.find_each(find_params) do |upload|
-          clean(upload.build_uploader, dry_run: dry_run)
+          clean(upload.retrieve_uploader, dry_run: dry_run)
           sleep sleep_time if sleep_time
         rescue => err
           logger.error "failed to sanitize #{upload_ref(upload)}: #{err.message}"

lib/gitlab/verify/uploads.rb

@@ -32,7 +32,7 @@ module Gitlab
       end
 
       def remote_object_exists?(upload)
-        upload.build_uploader.file.exists?
+        upload.retrieve_uploader.file.exists?
       end
     end
   end

spec/lib/gitlab/background_migration/legacy_upload_mover_spec.rb

@@ -32,7 +32,7 @@ describe Gitlab::BackgroundMigration::LegacyUploadMover do
 
     if with_file
       upload = create(:upload, :with_file, :attachment_upload, params)
-      model.update(attachment: upload.build_uploader)
+      model.update(attachment: upload.retrieve_uploader)
       model.attachment.upload
     else
       create(:upload, :attachment_upload, params)

spec/lib/gitlab/background_migration/legacy_uploads_migrator_spec.rb

@@ -24,7 +24,7 @@ describe Gitlab::BackgroundMigration::LegacyUploadsMigrator do
 
     if with_file
       upload = create(:upload, :with_file, :attachment_upload, params)
-      model.update(attachment: upload.build_uploader)
+      model.update(attachment: upload.retrieve_uploader)
       model.attachment.upload
     else
       create(:upload, :attachment_upload, params)

spec/lib/gitlab/import_export/uploads_manager_spec.rb

@@ -83,7 +83,7 @@ describe Gitlab::ImportExport::UploadsManager do
     it 'restores the file' do
       manager.restore
 
-      expect(project.uploads.map { |u| u.build_uploader.filename }).to include('dummy.txt')
+      expect(project.uploads.map { |u| u.retrieve_uploader.filename }).to include('dummy.txt')
     end
   end
 end

spec/lib/gitlab/import_export/uploads_restorer_spec.rb

@@ -27,7 +27,7 @@ describe Gitlab::ImportExport::UploadsRestorer do
       it 'copies the uploads to the project path' do
         subject.restore
 
-        expect(project.uploads.map { |u| u.build_uploader.filename }).to include('dummy.txt')
+        expect(project.uploads.map { |u| u.retrieve_uploader.filename }).to include('dummy.txt')
       end
     end
 
@@ -43,7 +43,7 @@ describe Gitlab::ImportExport::UploadsRestorer do
       it 'copies the uploads to the project path' do
         subject.restore
 
-        expect(project.uploads.map { |u| u.build_uploader.filename }).to include('dummy.txt')
+        expect(project.uploads.map { |u| u.retrieve_uploader.filename }).to include('dummy.txt')
       end
     end
   end

spec/lib/gitlab/sanitizers/exif_spec.rb

@@ -58,7 +58,7 @@ describe Gitlab::Sanitizers::Exif do
   end
 
   describe '#clean' do
-    let(:uploader) { create(:upload, :with_file, :issuable_upload).build_uploader }
+    let(:uploader) { create(:upload, :with_file, :issuable_upload).retrieve_uploader }
 
     context "no dry run" do
       it "removes exif from the image" do

spec/models/upload_spec.rb

@@ -107,6 +107,16 @@ describe Upload do
     end
   end
 
+  describe '#build_uploader' do
+    it 'returns a uploader object with current upload associated with it' do
+      subject = build(:upload)
+      uploader = subject.build_uploader
+
+      expect(uploader.upload).to eq(subject)
+      expect(uploader.mounted_as).to eq(subject.send(:mount_point))
+    end
+  end
+
   describe '#needs_checksum??' do
     context 'with local storage' do
       it 'returns true when no checksum exists' do

spec/models/uploads/fog_spec.rb

@@ -44,7 +44,7 @@ describe Uploads::Fog do
       subject { data_store.delete_keys(keys) }
 
       before do
-        uploads.each { |upload| upload.build_uploader.migrate!(2) }
+        uploads.each { |upload| upload.retrieve_uploader.migrate!(2) }
       end
 
       it 'deletes multiple data' do

spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb

@@ -104,7 +104,7 @@ shared_examples 'handle uploads' do
 
         context "when neither the uploader nor the model exists" do
           before do
-            allow_any_instance_of(Upload).to receive(:build_uploader).and_return(nil)
+            allow_any_instance_of(Upload).to receive(:retrieve_uploader).and_return(nil)
             allow(controller).to receive(:find_model).and_return(nil)
           end