Merge branch 'sh-fix-personal-snippet-uploads-object-storage' into 'master'

Fix incorrect prefix used in new uploads for personal snippets

Closes #61671

See merge request gitlab-org/gitlab-ce!28337

app/uploaders/file_uploader.rb

@@ -109,12 +109,20 @@ class FileUploader < GitlabUploader
   def upload_path
     if file_storage?
       # Legacy path relative to project.full_path
-      File.join(dynamic_segment, identifier)
+      local_storage_path(identifier)
     else
-      File.join(store_dir, identifier)
+      remote_storage_path(identifier)
     end
   end
 
+  def local_storage_path(file_identifier)
+    File.join(dynamic_segment, file_identifier)
+  end
+
+  def remote_storage_path(file_identifier)
+    File.join(store_dir, file_identifier)
+  end
+
   def store_dirs
     {
       Store::LOCAL => File.join(base_dir, dynamic_segment),

app/uploaders/personal_file_uploader.rb

@@ -6,15 +6,12 @@ class PersonalFileUploader < FileUploader
     options.storage_path
   end
 
-  def self.base_dir(model, store = nil)
-    base_dirs(model)[store || Store::LOCAL]
-  end
-
-  def self.base_dirs(model)
-    {
-      Store::LOCAL => File.join(options.base_dir, model_path_segment(model)),
-      Store::REMOTE => model_path_segment(model)
-    }
+  def self.base_dir(model, _store = nil)
+    # base_dir is the path seen by the user when rendering Markdown, so
+    # it should be the same for both local and object storage. It is
+    # typically prefaced with uploads/-/system, but that prefix
+    # is omitted in the path stored on disk.
+    File.join(options.base_dir, model_path_segment(model))
   end
 
   def self.model_path_segment(model)
@@ -40,8 +37,61 @@ class PersonalFileUploader < FileUploader
     store_dirs[object_store]
   end
 
+  # A personal snippet path is stored using FileUploader#upload_path.
+  #
+  # The format for the path:
+  #
+  # Local storage: :random_hex/:filename.
+  # Object storage: personal_snippet/:id/:random_hex/:filename.
+  #
+  # upload_paths represent the possible paths for a given identifier,
+  # which will vary depending on whether the file is stored in local or
+  # object storage. upload_path should match an element in upload_paths.
+  #
+  # base_dir represents the path seen by the user in Markdown, and it
+  # should always be prefixed with uploads/-/system.
+  #
+  # store_dirs represent the paths that are actually used on disk. For
+  # object storage, this should omit the prefix /uploads/-/system.
+  #
+  # For example, consider the requested path /uploads/-/system/personal_snippet/172/ff4ad5c2e40b39ae57cda51577317d20/file.png.
+  #
+  # For local storage:
+  #
+  # File on disk: /opt/gitlab/embedded/service/gitlab-rails/public/uploads/-/system/personal_snippet/172/ff4ad5c2e40b39ae57cda51577317d20/file.png.
+  #
+  # base_dir: uploads/-/system/personal_snippet/172
+  # upload_path: ff4ad5c2e40b39ae57cda51577317d20/file.png
+  # upload_paths: ["ff4ad5c2e40b39ae57cda51577317d20/file.png", "personal_snippet/172/ff4ad5c2e40b39ae57cda51577317d20/file.png"].
+  # store_dirs:
+  # => {1=>"uploads/-/system/personal_snippet/172/ff4ad5c2e40b39ae57cda51577317d20", 2=>"personal_snippet/172/ff4ad5c2e40b39ae57cda51577317d20"}
+  #
+  # For object storage:
+  #
+  # upload_path: personal_snippet/172/ff4ad5c2e40b39ae57cda51577317d20/file.png
+  def upload_paths(identifier)
+    [
+      local_storage_path(identifier),
+      File.join(remote_storage_base_path, identifier)
+    ]
+  end
+
+  def store_dirs
+    {
+      Store::LOCAL => File.join(base_dir, dynamic_segment),
+      Store::REMOTE => remote_storage_base_path
+    }
+  end
+
   private
 
+  # To avoid prefacing the remote storage path with `/uploads/-/system`,
+  # we just drop that part so that the destination path will be
+  # personal_snippet/:id/:random_hex/:filename.
+  def remote_storage_base_path
+    File.join(self.class.model_path_segment(model), dynamic_segment)
+  end
+
   def secure_url
     File.join('/', base_dir, secret, file.filename)
   end

changelogs/unreleased/sh-fix-personal-snippet-uploads-object-storage.yml

+---
+title: Fix incorrect prefix used in new uploads for personal snippets
+merge_request: 28337
+author:
+type: fixed

spec/uploaders/personal_file_uploader_spec.rb

@@ -7,33 +7,19 @@ describe PersonalFileUploader do
 
   subject { uploader }
 
-  it_behaves_like 'builds correct paths',
-                  store_dir: %r[uploads/-/system/personal_snippet/\d+],
-                  upload_path: %r[\h+/\S+],
-                  absolute_path: %r[#{CarrierWave.root}/uploads/-/system/personal_snippet\/\d+\/\h+\/\S+$]
-
-  context "object_store is REMOTE" do
+  shared_examples '#base_dir' do
     before do
-      stub_uploads_object_storage
-    end
-
-    include_context 'with storage', described_class::Store::REMOTE
-
-    it_behaves_like 'builds correct paths',
-                    store_dir: %r[\d+/\h+],
-                    upload_path: %r[^personal_snippet\/\d+\/\h+\/<filename>]
+      subject.instance_variable_set(:@secret, 'secret')
     end
 
-  describe '#upload_paths' do
-    it 'builds correct paths for both local and remote storage' do
-      paths = uploader.upload_paths('test.jpg')
+    it 'is prefixed with uploads/-/system' do
+      allow(uploader).to receive(:file).and_return(double(extension: 'txt', filename: 'file_name'))
 
-      expect(paths.first).to match(%r[\h+\/test.jpg])
-      expect(paths.second).to match(%r[^personal_snippet\/\d+\/\h+\/test.jpg])
+      expect(described_class.base_dir(model)).to eq("uploads/-/system/personal_snippet/#{model.id}")
     end
   end
 
-  describe '#to_h' do
+  shared_examples '#to_h' do
     before do
       subject.instance_variable_set(:@secret, 'secret')
     end
@@ -50,6 +36,40 @@ describe PersonalFileUploader do
     end
   end
 
+  describe '#upload_paths' do
+    it 'builds correct paths for both local and remote storage' do
+      paths = uploader.upload_paths('test.jpg')
+
+      expect(paths.first).to match(%r[\h+\/test.jpg])
+      expect(paths.second).to match(%r[^personal_snippet\/\d+\/\h+\/test.jpg])
+    end
+  end
+
+  context 'object_store is LOCAL' do
+    it_behaves_like 'builds correct paths',
+                    store_dir: %r[uploads/-/system/personal_snippet/\d+/\h+],
+                    upload_path: %r[\h+/\S+],
+                    absolute_path: %r[#{CarrierWave.root}/uploads/-/system/personal_snippet\/\d+\/\h+\/\S+$]
+
+    it_behaves_like '#base_dir'
+    it_behaves_like '#to_h'
+  end
+
+  context "object_store is REMOTE" do
+    before do
+      stub_uploads_object_storage
+    end
+
+    include_context 'with storage', described_class::Store::REMOTE
+
+    it_behaves_like 'builds correct paths',
+                    store_dir: %r[\d+/\h+],
+                    upload_path: %r[^personal_snippet\/\d+\/\h+\/<filename>]
+
+    it_behaves_like '#base_dir'
+    it_behaves_like '#to_h'
+  end
+
   describe "#migrate!" do
     before do
       uploader.store!(fixture_file_upload('spec/fixtures/doc_sample.txt'))