Handle relative and absolute Upload paths in the Uploaders

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'))