From e61f66b3d16cf097af8fbf3072018fd7d9ec8b67 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Tue, 26 Jun 2018 17:05:48 -0700 Subject: When moving issues, don't attempt to move files in object storage Closes #48505 --- .../sh-fix-move-issue-with-object-storage.yml | 5 +++++ lib/gitlab/gfm/uploads_rewriter.rb | 2 +- spec/lib/gitlab/gfm/uploads_rewriter_spec.rb | 19 +++++++++++++++++++ 3 files changed, 25 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/sh-fix-move-issue-with-object-storage.yml diff --git a/changelogs/unreleased/sh-fix-move-issue-with-object-storage.yml b/changelogs/unreleased/sh-fix-move-issue-with-object-storage.yml new file mode 100644 index 00000000000..e2df15a2847 --- /dev/null +++ b/changelogs/unreleased/sh-fix-move-issue-with-object-storage.yml @@ -0,0 +1,5 @@ +--- +title: When moving issues, don't attempt to move files in object storage +merge_request: +author: +type: fixed diff --git a/lib/gitlab/gfm/uploads_rewriter.rb b/lib/gitlab/gfm/uploads_rewriter.rb index b6eeb5d9a2b..ac00f3e2f8d 100644 --- a/lib/gitlab/gfm/uploads_rewriter.rb +++ b/lib/gitlab/gfm/uploads_rewriter.rb @@ -48,7 +48,7 @@ module Gitlab def find_file(project, secret, file) uploader = FileUploader.new(project, secret: secret) uploader.retrieve_from_store!(file) - uploader.file + uploader.file if uploader.object_store == ObjectStorage::Store::LOCAL end # Because the uploaders use 'move_to_store' we must have a temporary diff --git a/spec/lib/gitlab/gfm/uploads_rewriter_spec.rb b/spec/lib/gitlab/gfm/uploads_rewriter_spec.rb index 13df8531b63..4d72e60a8b3 100644 --- a/spec/lib/gitlab/gfm/uploads_rewriter_spec.rb +++ b/spec/lib/gitlab/gfm/uploads_rewriter_spec.rb @@ -62,5 +62,24 @@ describe Gitlab::Gfm::UploadsRewriter do subject { rewriter.files } it { is_expected.to be_an(Array) } end + + describe 'with object storage' do + before do + stub_uploads_object_storage(uploader: FileUploader) + zip_uploader.migrate!(FileUploader::Store::REMOTE) + end + + describe '#needs_rewrite?' do + subject { rewriter.needs_rewrite? } + + it { is_expected.to eq false } + end + + describe '#files' do + subject { rewriter.files } + + it { is_expected.to eq([]) } + end + end end end -- cgit v1.2.1 From cebdd267e672c75696cd534bb89d10fda8de129f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mica=C3=ABl=20Bergeron?= Date: Thu, 28 Jun 2018 10:57:28 -0400 Subject: add support for file copy on object storage --- app/uploaders/file_uploader.rb | 26 +++++++++++ lib/gitlab/gfm/uploads_rewriter.rb | 22 ++------- spec/lib/gitlab/gfm/uploads_rewriter_spec.rb | 68 ++++++++++++++++++---------- spec/uploaders/file_uploader_spec.rb | 44 ++++++++++++++++++ 4 files changed, 116 insertions(+), 44 deletions(-) diff --git a/app/uploaders/file_uploader.rb b/app/uploaders/file_uploader.rb index 36bc0a4575a..28399f1e051 100644 --- a/app/uploaders/file_uploader.rb +++ b/app/uploaders/file_uploader.rb @@ -81,6 +81,13 @@ class FileUploader < GitlabUploader apply_context!(uploader_context) end + def initialize_copy(from) + super + + @secret = self.class.generate_secret + @upload = nil # calling record_upload would delete the old upload if set + end + # enforce the usage of Hashed storage when storing to # remote store as the FileMover doesn't support OS def base_dir(store = nil) @@ -144,6 +151,25 @@ class FileUploader < GitlabUploader @secret ||= self.class.generate_secret end + # return a new uploader with a file copy on another project + def self.copy_to(uploader, to_project) + moved = uploader.dup.tap do |u| + u.model = to_project + end + + moved.copy_file(uploader.file) + moved + end + + def copy_file(file) + if file_storage? + store!(file) + else + self.file = file.copy_to(store_path) + record_upload # after_store is not triggered + end + end + private def apply_context!(uploader_context) diff --git a/lib/gitlab/gfm/uploads_rewriter.rb b/lib/gitlab/gfm/uploads_rewriter.rb index ac00f3e2f8d..f7e66697da3 100644 --- a/lib/gitlab/gfm/uploads_rewriter.rb +++ b/lib/gitlab/gfm/uploads_rewriter.rb @@ -23,11 +23,8 @@ module Gitlab file = find_file(@source_project, $~[:secret], $~[:file]) break markdown unless file.try(:exists?) - new_uploader = FileUploader.new(target_project) - with_link_in_tmp_dir(file.file) do |open_tmp_file| - new_uploader.store!(open_tmp_file) - end - new_uploader.markdown_link + moved = FileUploader.copy_to(file, target_project) + moved.markdown_link end end @@ -48,20 +45,7 @@ module Gitlab def find_file(project, secret, file) uploader = FileUploader.new(project, secret: secret) uploader.retrieve_from_store!(file) - uploader.file if uploader.object_store == ObjectStorage::Store::LOCAL - end - - # Because the uploaders use 'move_to_store' we must have a temporary - # file that is allowed to be (re)moved. - def with_link_in_tmp_dir(file) - dir = Dir.mktmpdir('UploadsRewriter', File.dirname(file)) - # The filename matters to Carrierwave so we make sure to preserve it - tmp_file = File.join(dir, File.basename(file)) - File.link(file, tmp_file) - # Open the file to placate Carrierwave - File.open(tmp_file) { |open_file| yield open_file } - ensure - FileUtils.rm_rf(dir) + uploader end end end diff --git a/spec/lib/gitlab/gfm/uploads_rewriter_spec.rb b/spec/lib/gitlab/gfm/uploads_rewriter_spec.rb index 4d72e60a8b3..9a3e958515f 100644 --- a/spec/lib/gitlab/gfm/uploads_rewriter_spec.rb +++ b/spec/lib/gitlab/gfm/uploads_rewriter_spec.rb @@ -20,37 +20,55 @@ describe Gitlab::Gfm::UploadsRewriter do "Text and #{image_uploader.markdown_link} and #{zip_uploader.markdown_link}" end - describe '#rewrite' do - let!(:new_text) { rewriter.rewrite(new_project) } - - let(:old_files) { [image_uploader, zip_uploader].map(&:file) } - let(:new_files) do - described_class.new(new_text, new_project, user).files + shared_examples "files are accessible" do + describe '#rewrite' do + let!(:new_text) { rewriter.rewrite(new_project) } + + let(:old_files) { [image_uploader, zip_uploader] } + let(:new_files) do + described_class.new(new_text, new_project, user).files + end + + let(:old_paths) { old_files.map(&:path) } + let(:new_paths) { new_files.map(&:path) } + + it 'rewrites content' do + expect(new_text).not_to eq text + expect(new_text.length).to eq text.length + end + + it 'copies files' do + expect(new_files).to all(exist) + expect(old_paths).not_to match_array new_paths + expect(old_paths).to all(include(old_project.disk_path)) + expect(new_paths).to all(include(new_project.disk_path)) + end + + it 'does not remove old files' do + expect(old_files).to all(exist) + end + + it 'generates a new secret for each file' do + expect(new_paths).not_to include image_uploader.secret + expect(new_paths).not_to include zip_uploader.secret + end end + end - let(:old_paths) { old_files.map(&:path) } - let(:new_paths) { new_files.map(&:path) } - - it 'rewrites content' do - expect(new_text).not_to eq text - expect(new_text.length).to eq text.length - end + context "file are stored locally" do + include_examples "files are accessible" + end - it 'copies files' do - expect(new_files).to all(exist) - expect(old_paths).not_to match_array new_paths - expect(old_paths).to all(include(old_project.disk_path)) - expect(new_paths).to all(include(new_project.disk_path)) - end + context "files are store remotely" do + before do + stub_uploads_object_storage(FileUploader) - it 'does not remove old files' do - expect(old_files).to all(exist) + old_files.each do |file| + file.migrate!(ObjectStorage::Store::REMOTE) + end end - it 'generates a new secret for each file' do - expect(new_paths).not_to include image_uploader.secret - expect(new_paths).not_to include zip_uploader.secret - end + include_examples "files are accessible" end describe '#needs_rewrite?' do diff --git a/spec/uploaders/file_uploader_spec.rb b/spec/uploaders/file_uploader_spec.rb index 59013a02938..1206b94b635 100644 --- a/spec/uploaders/file_uploader_spec.rb +++ b/spec/uploaders/file_uploader_spec.rb @@ -80,6 +80,50 @@ describe FileUploader do end end + describe 'copy_to' do + shared_examples 'returns a valid uploader' do + describe 'returned uploader' do + let(:new_project) { create(:project) } + let(:moved) { described_class.copy_to(subject, new_project) } + + it 'generates a new secret' do + expect(subject).to be + expect(described_class).to receive(:generate_secret).once.and_call_original + expect(moved).to be + end + + it 'create new upload' do + expect(moved.upload).not_to eq(subject.upload) + end + + it 'copies the file' do + expect(subject.file).to exist + expect(moved.file).to exist + expect(subject.file).not_to eq(moved.file) + expect(subject.object_store).to eq(moved.object_store) + end + end + end + + context 'files are store locally' do + include_examples 'returns a valid uploader' + + before do + subject.store!(fixture_file_upload('spec/fixtures/dk.png')) + end + end + + context 'files are stored remotely' do + before do + stub_uploads_object_storage + subject.store!(fixture_file_upload('spec/fixtures/dk.png')) + subject.migrate!(ObjectStorage::Store::REMOTE) + end + + include_examples 'returns a valid uploader' + end + end + describe '#secret' do it 'generates a secret if none is provided' do expect(described_class).to receive(:generate_secret).and_return('secret') -- cgit v1.2.1 From 91c9c4b72888f95fc07d14a008ec48c8114b4e41 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mica=C3=ABl=20Bergeron?= Date: Thu, 28 Jun 2018 11:25:40 -0400 Subject: fix an issue with local files --- app/uploaders/file_uploader.rb | 14 ++++++++------ spec/lib/gitlab/gfm/uploads_rewriter_spec.rb | 19 ------------------- 2 files changed, 8 insertions(+), 25 deletions(-) diff --git a/app/uploaders/file_uploader.rb b/app/uploaders/file_uploader.rb index 28399f1e051..73606eb9f83 100644 --- a/app/uploaders/file_uploader.rb +++ b/app/uploaders/file_uploader.rb @@ -162,12 +162,14 @@ class FileUploader < GitlabUploader end def copy_file(file) - if file_storage? - store!(file) - else - self.file = file.copy_to(store_path) - record_upload # after_store is not triggered - end + to_path = if file_storage? + File.join(self.class.root, store_path) + else + store_path + end + + self.file = file.copy_to(to_path) + record_upload # after_store is not triggered end private diff --git a/spec/lib/gitlab/gfm/uploads_rewriter_spec.rb b/spec/lib/gitlab/gfm/uploads_rewriter_spec.rb index 9a3e958515f..bf42c583499 100644 --- a/spec/lib/gitlab/gfm/uploads_rewriter_spec.rb +++ b/spec/lib/gitlab/gfm/uploads_rewriter_spec.rb @@ -80,24 +80,5 @@ describe Gitlab::Gfm::UploadsRewriter do subject { rewriter.files } it { is_expected.to be_an(Array) } end - - describe 'with object storage' do - before do - stub_uploads_object_storage(uploader: FileUploader) - zip_uploader.migrate!(FileUploader::Store::REMOTE) - end - - describe '#needs_rewrite?' do - subject { rewriter.needs_rewrite? } - - it { is_expected.to eq false } - end - - describe '#files' do - subject { rewriter.files } - - it { is_expected.to eq([]) } - end - end end end -- cgit v1.2.1 From ad1241b6b7e62a26455218199496c3eb958dac00 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mica=C3=ABl=20Bergeron?= Date: Fri, 29 Jun 2018 00:09:57 +0000 Subject: fix the changelog --- changelogs/unreleased/sh-fix-move-issue-with-object-storage.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/changelogs/unreleased/sh-fix-move-issue-with-object-storage.yml b/changelogs/unreleased/sh-fix-move-issue-with-object-storage.yml index e2df15a2847..9c5c4ca20d8 100644 --- a/changelogs/unreleased/sh-fix-move-issue-with-object-storage.yml +++ b/changelogs/unreleased/sh-fix-move-issue-with-object-storage.yml @@ -1,5 +1,5 @@ --- -title: When moving issues, don't attempt to move files in object storage -merge_request: +title: Implement upload copy when moving an issue with upload on object storage +merge_request: 20191 author: type: fixed -- cgit v1.2.1 From 1181cf336f7def94b7a00c9b631f7ef0b5e74bba Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Tue, 3 Jul 2018 09:54:54 -0700 Subject: Fix minor spec review comments in uploader specs --- spec/lib/gitlab/gfm/uploads_rewriter_spec.rb | 2 +- spec/uploaders/file_uploader_spec.rb | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/spec/lib/gitlab/gfm/uploads_rewriter_spec.rb b/spec/lib/gitlab/gfm/uploads_rewriter_spec.rb index bf42c583499..ef52a25f47e 100644 --- a/spec/lib/gitlab/gfm/uploads_rewriter_spec.rb +++ b/spec/lib/gitlab/gfm/uploads_rewriter_spec.rb @@ -59,7 +59,7 @@ describe Gitlab::Gfm::UploadsRewriter do include_examples "files are accessible" end - context "files are store remotely" do + context "files are stored remotely" do before do stub_uploads_object_storage(FileUploader) diff --git a/spec/uploaders/file_uploader_spec.rb b/spec/uploaders/file_uploader_spec.rb index 1206b94b635..7ba28b4fc1f 100644 --- a/spec/uploaders/file_uploader_spec.rb +++ b/spec/uploaders/file_uploader_spec.rb @@ -105,12 +105,12 @@ describe FileUploader do end end - context 'files are store locally' do - include_examples 'returns a valid uploader' - + context 'files are stored locally' do before do subject.store!(fixture_file_upload('spec/fixtures/dk.png')) end + + include_examples 'returns a valid uploader' end context 'files are stored remotely' do -- cgit v1.2.1