diff options
author | Stan Hu <stanhu@gmail.com> | 2019-05-15 09:26:18 -0700 |
---|---|---|
committer | Stan Hu <stanhu@gmail.com> | 2019-05-15 11:29:51 -0700 |
commit | 23381841759d6764c4351b4177d1a09a6c33bee3 (patch) | |
tree | 5aa111a733f6c0a682120326e107d3544bf8c5ea | |
parent | 568999e6886db4c94d21d974b49f1f89e547e013 (diff) | |
download | gitlab-ce-sh-fix-personal-snippet-uploads-object-storage.tar.gz |
Fix incorrect prefix used in new uploads for personal snippetssh-fix-personal-snippet-uploads-object-storage
https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/24550 fixed the
case where the wrong path on disk was being searched, but it
inadvertently ommitted the `/uploads/-/system` prefix when rendering the
Markdown for personal snippet uploads when they were stored directly in
object storage.
We now have the following scheme:
1. base_dir represents the path seen by the user in Markdown, and it
should always be prefixed with `/uploads/-/system.`
2. store_dirs represent the paths that are actually used on disk. For
object storage, this should omit the prefix `/uploads/-/system`.
3. upload_paths represent the paths that are used to search and should
match `store_dirs`. This also omits the prefix.
Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/61671
-rw-r--r-- | app/uploaders/personal_file_uploader.rb | 29 | ||||
-rw-r--r-- | changelogs/unreleased/sh-fix-personal-snippet-uploads-object-storage.yml | 5 | ||||
-rw-r--r-- | spec/uploaders/personal_file_uploader_spec.rb | 62 |
3 files changed, 67 insertions, 29 deletions
diff --git a/app/uploaders/personal_file_uploader.rb b/app/uploaders/personal_file_uploader.rb index 272837aa6ce..22e6f8d67bc 100644 --- a/app/uploaders/personal_file_uploader.rb +++ b/app/uploaders/personal_file_uploader.rb @@ -6,15 +6,12 @@ class PersonalFileUploader < FileUploader options.storage_path end - def self.base_dir(model, store = nil) - base_dirs(model)[store || Store::LOCAL] - end - - def self.base_dirs(model) - { - Store::LOCAL => File.join(options.base_dir, model_path_segment(model)), - Store::REMOTE => model_path_segment(model) - } + def self.base_dir(model, _store = nil) + # base_dir is the path seen by the user when rendering Markdown, so + # it should be the same for both local and object storage. It is + # typically prefaced with uploads/-/system, but that prefix + # is omitted in the path stored on disk. + File.join(options.base_dir, model_path_segment(model)) end def self.model_path_segment(model) @@ -40,6 +37,20 @@ class PersonalFileUploader < FileUploader store_dirs[object_store] end + def upload_paths(identifier) + [ + File.join(dynamic_segment, identifier), + File.join(self.class.model_path_segment(model), dynamic_segment, identifier) + ] + end + + def store_dirs + { + Store::LOCAL => File.join(base_dir, dynamic_segment), + Store::REMOTE => File.join(self.class.model_path_segment(model), dynamic_segment) + } + end + private def secure_url diff --git a/changelogs/unreleased/sh-fix-personal-snippet-uploads-object-storage.yml b/changelogs/unreleased/sh-fix-personal-snippet-uploads-object-storage.yml new file mode 100644 index 00000000000..603afa8573f --- /dev/null +++ b/changelogs/unreleased/sh-fix-personal-snippet-uploads-object-storage.yml @@ -0,0 +1,5 @@ +--- +title: Fix incorrect prefix used in new uploads for personal snippets +merge_request: 28337 +author: +type: fixed diff --git a/spec/uploaders/personal_file_uploader_spec.rb b/spec/uploaders/personal_file_uploader_spec.rb index 97758f0243e..44e7d7a94ee 100644 --- a/spec/uploaders/personal_file_uploader_spec.rb +++ b/spec/uploaders/personal_file_uploader_spec.rb @@ -7,33 +7,19 @@ describe PersonalFileUploader do subject { uploader } - it_behaves_like 'builds correct paths', - store_dir: %r[uploads/-/system/personal_snippet/\d+], - upload_path: %r[\h+/\S+], - absolute_path: %r[#{CarrierWave.root}/uploads/-/system/personal_snippet\/\d+\/\h+\/\S+$] - - context "object_store is REMOTE" do + shared_examples '#base_dir' do before do - stub_uploads_object_storage + subject.instance_variable_set(:@secret, 'secret') end - include_context 'with storage', described_class::Store::REMOTE - - it_behaves_like 'builds correct paths', - store_dir: %r[\d+/\h+], - upload_path: %r[^personal_snippet\/\d+\/\h+\/<filename>] - end - - describe '#upload_paths' do - it 'builds correct paths for both local and remote storage' do - paths = uploader.upload_paths('test.jpg') + it 'is prefixed with uploads/-/system' do + allow(uploader).to receive(:file).and_return(double(extension: 'txt', filename: 'file_name')) - expect(paths.first).to match(%r[\h+\/test.jpg]) - expect(paths.second).to match(%r[^personal_snippet\/\d+\/\h+\/test.jpg]) + expect(described_class.base_dir(model)).to eq("uploads/-/system/personal_snippet/#{model.id}") end end - describe '#to_h' do + shared_examples '#to_h' do before do subject.instance_variable_set(:@secret, 'secret') end @@ -50,6 +36,42 @@ describe PersonalFileUploader do end end + shared_examples '#upload_paths' do + it 'builds correct paths for both local and remote storage' do + paths = uploader.upload_paths('test.jpg') + + expect(paths.first).to match(%r[\h+\/test.jpg]) + expect(paths.second).to match(%r[^personal_snippet\/\d+\/\h+\/test.jpg]) + end + end + + context 'object_store is LOCAL' do + it_behaves_like 'builds correct paths', + store_dir: %r[uploads/-/system/personal_snippet/\d+], + upload_path: %r[\h+/\S+], + absolute_path: %r[#{CarrierWave.root}/uploads/-/system/personal_snippet\/\d+\/\h+\/\S+$] + + it_behaves_like '#base_dir' + it_behaves_like '#to_h' + it_behaves_like '#upload_paths' + end + + context "object_store is REMOTE" do + before do + stub_uploads_object_storage + end + + include_context 'with storage', described_class::Store::REMOTE + + it_behaves_like 'builds correct paths', + store_dir: %r[\d+/\h+], + upload_path: %r[^personal_snippet\/\d+\/\h+\/<filename>] + + it_behaves_like '#base_dir' + it_behaves_like '#to_h' + it_behaves_like '#upload_paths' + end + describe "#migrate!" do before do uploader.store!(fixture_file_upload('spec/fixtures/doc_sample.txt')) |