From cebdd267e672c75696cd534bb89d10fda8de129f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mica=C3=ABl=20Bergeron?= <mbergeron@gitlab.com>
Date: Thu, 28 Jun 2018 10:57:28 -0400
Subject: [PATCH] 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')
--
2.30.9