diff options
-rw-r--r-- | app/uploaders/file_uploader.rb | 6 | ||||
-rw-r--r-- | app/uploaders/object_storage.rb | 11 | ||||
-rw-r--r-- | spec/uploaders/workers/object_storage/migrate_uploads_worker_spec.rb | 50 |
3 files changed, 53 insertions, 14 deletions
diff --git a/app/uploaders/file_uploader.rb b/app/uploaders/file_uploader.rb index 133fdf6684d..36bc0a4575a 100644 --- a/app/uploaders/file_uploader.rb +++ b/app/uploaders/file_uploader.rb @@ -65,10 +65,10 @@ class FileUploader < GitlabUploader SecureRandom.hex end - def upload_paths(filename) + def upload_paths(identifier) [ - File.join(secret, filename), - File.join(base_dir(Store::REMOTE), secret, filename) + File.join(secret, identifier), + File.join(base_dir(Store::REMOTE), secret, identifier) ] end diff --git a/app/uploaders/object_storage.rb b/app/uploaders/object_storage.rb index 5aa1bc7227c..3f5d0d200f4 100644 --- a/app/uploaders/object_storage.rb +++ b/app/uploaders/object_storage.rb @@ -29,7 +29,7 @@ module ObjectStorage end def retrieve_from_store!(identifier) - paths = store_dirs.map { |store, path| File.join(path, identifier) } + paths = upload_paths(identifier) unless current_upload_satisfies?(paths, model) # the upload we already have isn't right, find the correct one @@ -261,7 +261,7 @@ module ObjectStorage end def delete_migrated_file(migrated_file) - migrated_file.delete if exists? + migrated_file.delete end def exists? @@ -279,6 +279,13 @@ module ObjectStorage } end + # Returns all the possible paths for an upload. + # the `upload.path` is a lookup parameter, and it may change + # depending on the `store` param. + def upload_paths(identifier) + store_dirs.map { |store, path| File.join(path, identifier) } + end + def cache!(new_file = sanitized_file) # We intercept ::UploadedFile which might be stored on remote storage # We use that for "accelerated" uploads, where we store result on remote storage diff --git a/spec/uploaders/workers/object_storage/migrate_uploads_worker_spec.rb b/spec/uploaders/workers/object_storage/migrate_uploads_worker_spec.rb index aed62f97448..18da0c6d39a 100644 --- a/spec/uploaders/workers/object_storage/migrate_uploads_worker_spec.rb +++ b/spec/uploaders/workers/object_storage/migrate_uploads_worker_spec.rb @@ -1,5 +1,7 @@ require 'spec_helper' +MIGRATION_QUERIES = 7 + describe ObjectStorage::MigrateUploadsWorker, :sidekiq do shared_context 'sanity_check! fails' do before do @@ -11,6 +13,13 @@ describe ObjectStorage::MigrateUploadsWorker, :sidekiq do let(:uploads) { Upload.all } let(:to_store) { ObjectStorage::Store::REMOTE } + def perform(uploads) + binding.pry + described_class.new.perform(uploads.ids, model_class.to_s, mounted_as, to_store) + rescue ObjectStorage::MigrateUploadsWorker::Report::MigrationFailures + # swallow + end + shared_examples "uploads migration worker" do describe '.enqueue!' do def enqueue! @@ -69,12 +78,6 @@ describe ObjectStorage::MigrateUploadsWorker, :sidekiq do end describe '#perform' do - def perform - described_class.new.perform(uploads.ids, model_class.to_s, mounted_as, to_store) - rescue ObjectStorage::MigrateUploadsWorker::Report::MigrationFailures - # swallow - end - shared_examples 'outputs correctly' do |success: 0, failures: 0| total = success + failures @@ -82,7 +85,7 @@ describe ObjectStorage::MigrateUploadsWorker, :sidekiq do it 'outputs the reports' do expect(Rails.logger).to receive(:info).with(%r{Migrated #{success}/#{total} files}) - perform + perform(uploads) end end @@ -90,7 +93,7 @@ describe ObjectStorage::MigrateUploadsWorker, :sidekiq do it 'outputs upload failures' do expect(Rails.logger).to receive(:warn).with(/Error .* I am a teapot/) - perform + perform(uploads) end end end @@ -98,7 +101,7 @@ describe ObjectStorage::MigrateUploadsWorker, :sidekiq do it_behaves_like 'outputs correctly', success: 10 it 'migrates files' do - perform + perform(uploads) expect(Upload.where(store: ObjectStorage::Store::LOCAL).count).to eq(0) end @@ -123,6 +126,18 @@ describe ObjectStorage::MigrateUploadsWorker, :sidekiq do end it_behaves_like "uploads migration worker" + + describe "limits N+1 queries" do + let!(:projects) { create_list(:project, 10, :with_avatar) } + + it do + query_count = ActiveRecord::QueryRecorder.new { perform(uploads) } + + more_projects = create_list(:project, 100, :with_avatar) + expected_queries_per_migration = MIGRATION_QUERIES * more_projects.count + expect { perform(Upload.all) }.not_to exceed_query_limit(query_count).with_threshold(expected_queries_per_migration) + end + end end context "for FileUploader" do @@ -140,5 +155,22 @@ describe ObjectStorage::MigrateUploadsWorker, :sidekiq do end it_behaves_like "uploads migration worker" + + describe "limits N+1 queries" do + let!(:projects) { create_list(:project, 10) } + + it do + query_count = ActiveRecord::QueryRecorder.new { perform(uploads) } + + more_projects = create_list(:project, 100) + more_projects.map do |project| + uploader = FileUploader.new(project) + uploader.store!(fixture_file_upload('spec/fixtures/doc_sample.txt')) + end + expected_queries_per_migration = MIGRATION_QUERIES * more_projects.count + + expect { perform(Upload.all) }.not_to exceed_query_limit(query_count).with_threshold(expected_queries_per_migration) + end + end end end |