Handle relative and absolute Upload paths in the Uploaders

3 files changed, 45 insertions, 6 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)