diff options
| -rw-r--r-- | app/controllers/concerns/uploads_actions.rb | 18 | ||||
| -rw-r--r-- | app/uploaders/file_uploader.rb | 48 | ||||
| -rw-r--r-- | app/uploaders/namespace_file_uploader.rb | 4 | ||||
| -rw-r--r-- | app/uploaders/personal_file_uploader.rb | 4 | ||||
| -rw-r--r-- | ee/changelogs/unreleased/poc-upload-hashing-path.yml | 5 | ||||
| -rw-r--r-- | spec/uploaders/file_uploader_spec.rb | 38 |
6 files changed, 70 insertions, 47 deletions
diff --git a/app/controllers/concerns/uploads_actions.rb b/app/controllers/concerns/uploads_actions.rb index 3dbfabcae8a..f4e19615760 100644 --- a/app/controllers/concerns/uploads_actions.rb +++ b/app/controllers/concerns/uploads_actions.rb @@ -62,19 +62,27 @@ module UploadsActions end def build_uploader_from_upload - return nil unless params[:secret] && params[:filename] + return unless uploader = build_uploader - upload_path = uploader_class.upload_path(params[:secret], params[:filename]) - upload = Upload.find_by(uploader: uploader_class.to_s, path: upload_path) + upload_paths = uploader.upload_paths(params[:filename]) + upload = Upload.find_by(uploader: uploader_class.to_s, path: upload_paths) upload&.build_uploader end def build_uploader_from_params + return unless uploader = build_uploader + + uploader.retrieve_from_store!(params[:filename]) + uploader + end + + def build_uploader + return unless params[:secret] && params[:filename] + uploader = uploader_class.new(model, secret: params[:secret]) - return nil unless uploader.model_valid? + return unless uploader.model_valid? - uploader.retrieve_from_store!(params[:filename]) uploader end diff --git a/app/uploaders/file_uploader.rb b/app/uploaders/file_uploader.rb index 0e2da64de6a..133fdf6684d 100644 --- a/app/uploaders/file_uploader.rb +++ b/app/uploaders/file_uploader.rb @@ -32,8 +32,11 @@ class FileUploader < GitlabUploader ) end - def self.base_dir(model) - model_path_segment(model) + def self.base_dir(model, store = Store::LOCAL) + decorated_model = model + decorated_model = Storage::HashedProject.new(model) if store == Store::REMOTE + + model_path_segment(decorated_model) end # used in migrations and import/exports @@ -51,21 +54,24 @@ class FileUploader < GitlabUploader # # Returns a String without a trailing slash def self.model_path_segment(model) - if model.hashed_storage?(:attachments) - model.disk_path + case model + when Storage::HashedProject then model.disk_path else - model.full_path + model.hashed_storage?(:attachments) ? model.disk_path : model.full_path end end - def self.upload_path(secret, identifier) - File.join(secret, identifier) - end - def self.generate_secret SecureRandom.hex end + def upload_paths(filename) + [ + File.join(secret, filename), + File.join(base_dir(Store::REMOTE), secret, filename) + ] + end + attr_accessor :model def initialize(model, mounted_as = nil, **uploader_context) @@ -75,8 +81,10 @@ class FileUploader < GitlabUploader apply_context!(uploader_context) end - def base_dir - self.class.base_dir(@model) + # enforce the usage of Hashed storage when storing to + # remote store as the FileMover doesn't support OS + def base_dir(store = nil) + self.class.base_dir(@model, store || object_store) end # we don't need to know the actual path, an uploader instance should be @@ -86,15 +94,19 @@ class FileUploader < GitlabUploader end def upload_path - self.class.upload_path(dynamic_segment, identifier) - end - - def model_path_segment - self.class.model_path_segment(@model) + if file_storage? + # Legacy path relative to project.full_path + File.join(dynamic_segment, identifier) + else + File.join(store_dir, identifier) + end end - def store_dir - File.join(base_dir, dynamic_segment) + def store_dirs + { + Store::LOCAL => File.join(base_dir, dynamic_segment), + Store::REMOTE => File.join(base_dir(ObjectStorage::Store::REMOTE), dynamic_segment) + } end def markdown_link diff --git a/app/uploaders/namespace_file_uploader.rb b/app/uploaders/namespace_file_uploader.rb index 269415b1926..1085ecb1700 100644 --- a/app/uploaders/namespace_file_uploader.rb +++ b/app/uploaders/namespace_file_uploader.rb @@ -4,7 +4,7 @@ class NamespaceFileUploader < FileUploader options.storage_path end - def self.base_dir(model) + def self.base_dir(model, _store = nil) File.join(options.base_dir, 'namespace', model_path_segment(model)) end @@ -20,7 +20,7 @@ class NamespaceFileUploader < FileUploader def store_dirs { Store::LOCAL => File.join(base_dir, dynamic_segment), - Store::REMOTE => File.join('namespace', model_path_segment, dynamic_segment) + Store::REMOTE => File.join('namespace', self.class.model_path_segment(model), dynamic_segment) } end end diff --git a/app/uploaders/personal_file_uploader.rb b/app/uploaders/personal_file_uploader.rb index d225002ad2e..e3898b07730 100644 --- a/app/uploaders/personal_file_uploader.rb +++ b/app/uploaders/personal_file_uploader.rb @@ -4,7 +4,7 @@ class PersonalFileUploader < FileUploader options.storage_path end - def self.base_dir(model) + def self.base_dir(model, _store = nil) File.join(options.base_dir, model_path_segment(model)) end @@ -34,7 +34,7 @@ class PersonalFileUploader < FileUploader def store_dirs { Store::LOCAL => File.join(base_dir, dynamic_segment), - Store::REMOTE => File.join(model_path_segment, dynamic_segment) + Store::REMOTE => File.join(self.class.model_path_segment(model), dynamic_segment) } end diff --git a/ee/changelogs/unreleased/poc-upload-hashing-path.yml b/ee/changelogs/unreleased/poc-upload-hashing-path.yml new file mode 100644 index 00000000000..7970405bea1 --- /dev/null +++ b/ee/changelogs/unreleased/poc-upload-hashing-path.yml @@ -0,0 +1,5 @@ +--- +title: File uploads in remote storage now support project renaming. +merge_request: 4597 +author: +type: fixed diff --git a/spec/uploaders/file_uploader_spec.rb b/spec/uploaders/file_uploader_spec.rb index 4b7dcbeda33..db2810bbe1d 100644 --- a/spec/uploaders/file_uploader_spec.rb +++ b/spec/uploaders/file_uploader_spec.rb @@ -11,34 +11,30 @@ describe FileUploader do shared_examples 'builds correct legacy storage paths' do include_examples 'builds correct paths', store_dir: %r{awesome/project/\h+}, + upload_path: %r{\h+/<filename>}, absolute_path: %r{#{described_class.root}/awesome/project/secret/foo.jpg} end - shared_examples 'uses hashed storage' do - context 'when rolled out attachments' do - let(:project) { build_stubbed(:project, namespace: group, name: 'project') } + context 'legacy storage' do + it_behaves_like 'builds correct legacy storage paths' - before do - allow(project).to receive(:disk_path).and_return('ca/fe/fe/ed') - end + context 'uses hashed storage' do + context 'when rolled out attachments' do + let(:project) { build_stubbed(:project, namespace: group, name: 'project') } - it_behaves_like 'builds correct paths', - store_dir: %r{ca/fe/fe/ed/\h+}, - absolute_path: %r{#{described_class.root}/ca/fe/fe/ed/secret/foo.jpg} - end + include_examples 'builds correct paths', + store_dir: %r{@hashed/\h{2}/\h{2}/\h+}, + upload_path: %r{\h+/<filename>} + end - context 'when only repositories are rolled out' do - let(:project) { build_stubbed(:project, namespace: group, name: 'project', storage_version: Project::HASHED_STORAGE_FEATURES[:repository]) } + context 'when only repositories are rolled out' do + let(:project) { build_stubbed(:project, namespace: group, name: 'project', storage_version: Project::HASHED_STORAGE_FEATURES[:repository]) } - it_behaves_like 'builds correct legacy storage paths' + it_behaves_like 'builds correct legacy storage paths' + end end end - context 'legacy storage' do - it_behaves_like 'builds correct legacy storage paths' - include_examples 'uses hashed storage' - end - context 'object store is remote' do before do stub_uploads_object_storage @@ -46,8 +42,10 @@ describe FileUploader do include_context 'with storage', described_class::Store::REMOTE - it_behaves_like 'builds correct legacy storage paths' - include_examples 'uses hashed storage' + # always use hashed storage path for remote uploads + it_behaves_like 'builds correct paths', + store_dir: %r{@hashed/\h{2}/\h{2}/\h+}, + upload_path: %r{@hashed/\h{2}/\h{2}/\h+/\h+/<filename>} end describe 'initialize' do |