diff options
| -rw-r--r-- | app/uploaders/file_uploader.rb | 30 | ||||
| -rw-r--r-- | app/uploaders/gitlab_uploader.rb | 15 | ||||
| -rw-r--r-- | app/uploaders/records_uploads.rb | 6 | ||||
| -rw-r--r-- | spec/uploaders/file_uploader_spec.rb | 21 | ||||
| -rw-r--r-- | spec/uploaders/records_uploads_spec.rb | 8 |
5 files changed, 70 insertions, 10 deletions
diff --git a/app/uploaders/file_uploader.rb b/app/uploaders/file_uploader.rb index 2cf97a1b6fd..d6ccf0dc92c 100644 --- a/app/uploaders/file_uploader.rb +++ b/app/uploaders/file_uploader.rb @@ -6,6 +6,26 @@ class FileUploader < GitlabUploader storage :file + def self.absolute_path(upload_record) + File.join( + self.dynamic_path_segment(upload_record.model), + upload_record.path + ) + end + + # Returns the part of `store_dir` that can change based on the model's current + # path + # + # This is used to build Upload paths dynamically based on the model's current + # namespace and path, allowing us to ignore renames or transfers. + # + # model - Object that responds to `path_with_namespace` + # + # Returns a String without a trailing slash + def self.dynamic_path_segment(model) + File.join(CarrierWave.root, base_dir, model.path_with_namespace) + end + attr_accessor :project attr_reader :secret @@ -15,7 +35,7 @@ class FileUploader < GitlabUploader end def store_dir - File.join(base_dir, @project.path_with_namespace, @secret) + File.join(dynamic_path_segment, @secret) end def cache_dir @@ -26,6 +46,10 @@ class FileUploader < GitlabUploader project end + def relative_path + self.file.path.sub("#{dynamic_path_segment}/", '') + end + def to_markdown to_h[:markdown] end @@ -46,6 +70,10 @@ class FileUploader < GitlabUploader private + def dynamic_path_segment + self.class.dynamic_path_segment(model) + end + def generate_secret SecureRandom.hex end diff --git a/app/uploaders/gitlab_uploader.rb b/app/uploaders/gitlab_uploader.rb index bd7de4ed562..d662ba6820c 100644 --- a/app/uploaders/gitlab_uploader.rb +++ b/app/uploaders/gitlab_uploader.rb @@ -1,4 +1,8 @@ class GitlabUploader < CarrierWave::Uploader::Base + def self.absolute_path(upload_record) + File.join(CarrierWave.root, upload_record.path) + end + def self.base_dir 'uploads' end @@ -18,4 +22,15 @@ class GitlabUploader < CarrierWave::Uploader::Base def move_to_store true end + + # Designed to be overridden by child uploaders that have a dynamic path + # segment -- that is, a path that changes based on mutable attributes of its + # associated model + # + # For example, `FileUploader` builds the storage path based on the associated + # project model's `path_with_namespace` value, which can change when the + # project or its containing namespace is moved or renamed. + def relative_path + self.file.path.sub("#{root}/", '') + end end diff --git a/app/uploaders/records_uploads.rb b/app/uploaders/records_uploads.rb index 7a0424b5adf..4c127f29250 100644 --- a/app/uploaders/records_uploads.rb +++ b/app/uploaders/records_uploads.rb @@ -22,11 +22,7 @@ module RecordsUploads Upload.record(self) end - # When removing an attachment, destroy any Upload records at the same path - # - # Note: this _will not work_ for Uploaders which relativize paths, such as - # `FileUploader`, but because that uploader uses different paths for every - # upload, that's an acceptable caveat. + # Before removing an attachment, destroy any Upload records at the same path # # Called `before :remove` def destroy_upload(*args) diff --git a/spec/uploaders/file_uploader_spec.rb b/spec/uploaders/file_uploader_spec.rb index 396b0abdaef..d9113ef4095 100644 --- a/spec/uploaders/file_uploader_spec.rb +++ b/spec/uploaders/file_uploader_spec.rb @@ -3,6 +3,18 @@ require 'spec_helper' describe FileUploader do let(:uploader) { described_class.new(build_stubbed(:empty_project)) } + describe '.absolute_path' do + it 'returns the correct absolute path by building it dynamically' do + project = build_stubbed(:project) + upload = double(model: project, path: 'secret/foo.jpg') + + dynamic_segment = project.path_with_namespace + + expect(described_class.absolute_path(upload)) + .to end_with("#{dynamic_segment}/secret/foo.jpg") + end + end + describe 'initialize' do it 'generates a secret if none is provided' do expect(SecureRandom).to receive(:hex).and_return('secret') @@ -32,4 +44,13 @@ describe FileUploader do expect(uploader.move_to_store).to eq(true) end end + + describe '#relative_path' do + it 'removes the leading dynamic path segment' do + fixture = Rails.root.join('spec', 'fixtures', 'rails_sample.jpg') + uploader.store!(fixture_file_upload(fixture)) + + expect(uploader.relative_path).to match(/\A\h{32}\/rails_sample.jpg\z/) + end + end end diff --git a/spec/uploaders/records_uploads_spec.rb b/spec/uploaders/records_uploads_spec.rb index a104dc4257f..5c26e334a6e 100644 --- a/spec/uploaders/records_uploads_spec.rb +++ b/spec/uploaders/records_uploads_spec.rb @@ -2,7 +2,7 @@ require 'rails_helper' describe RecordsUploads do let(:uploader) do - example_uploader = Class.new(GitlabUploader) do + class RecordsUploadsExampleUploader < GitlabUploader include RecordsUploads storage :file @@ -12,7 +12,7 @@ describe RecordsUploads do end end - example_uploader.new + RecordsUploadsExampleUploader.new end def upload_fixture(filename) @@ -59,10 +59,10 @@ describe RecordsUploads do it 'it destroys Upload records at the same path before recording' do existing = Upload.create!( - path: File.join(CarrierWave.root, 'uploads', 'rails_sample.jpg'), + path: File.join('uploads', 'rails_sample.jpg'), size: 512.kilobytes, model: build_stubbed(:user), - uploader: described_class.to_s + uploader: uploader.class.to_s ) uploader.store!(upload_fixture('rails_sample.jpg')) |