Merge branch 'poc-upload-hashing-path' into 'master'

File uploads on objects storage should use hashed storage

Closes #4952

See merge request gitlab-org/gitlab-ee!4597

app/controllers/concerns/uploads_actions.rb

@@ -62,19 +62,27 @@ module UploadsActions
   end
 
   def build_uploader_from_upload
-    return nil unless params[:secret] && params[:filename]
+    return unless uploader = build_uploader
 
-    upload_path = uploader_class.upload_path(params[:secret], params[:filename])
-    upload = Upload.find_by(uploader: uploader_class.to_s, path: upload_path)
+    upload_paths = uploader.upload_paths(params[:filename])
+    upload = Upload.find_by(uploader: uploader_class.to_s, path: upload_paths)
     upload&.build_uploader
   end
 
   def build_uploader_from_params
+    return unless uploader = build_uploader
+
+    uploader.retrieve_from_store!(params[:filename])
+    uploader
+  end
+
+  def build_uploader
+    return unless params[:secret] && params[:filename]
+
     uploader = uploader_class.new(model, secret: params[:secret])
 
-    return nil unless uploader.model_valid?
+    return unless uploader.model_valid?
 
-    uploader.retrieve_from_store!(params[:filename])
     uploader
   end
 

app/uploaders/file_uploader.rb

@@ -32,8 +32,11 @@ class FileUploader < GitlabUploader
     )
   end
 
-  def self.base_dir(model)
-    model_path_segment(model)
+  def self.base_dir(model, store = Store::LOCAL)
+    decorated_model = model
+    decorated_model = Storage::HashedProject.new(model) if store == Store::REMOTE
+
+    model_path_segment(decorated_model)
   end
 
   # used in migrations and import/exports
@@ -51,21 +54,24 @@ class FileUploader < GitlabUploader
   #
   # Returns a String without a trailing slash
   def self.model_path_segment(model)
-    if model.hashed_storage?(:attachments)
-      model.disk_path
+    case model
+    when Storage::HashedProject then model.disk_path
     else
-      model.full_path
-    end
+      model.hashed_storage?(:attachments) ? model.disk_path : model.full_path
     end
-
-  def self.upload_path(secret, identifier)
-    File.join(secret, identifier)
   end
 
   def self.generate_secret
     SecureRandom.hex
   end
 
+  def upload_paths(filename)
+    [
+      File.join(secret, filename),
+      File.join(base_dir(Store::REMOTE), secret, filename)
+    ]
+  end
+
   attr_accessor :model
 
   def initialize(model, mounted_as = nil, **uploader_context)
@@ -75,8 +81,10 @@ class FileUploader < GitlabUploader
     apply_context!(uploader_context)
   end
 
-  def base_dir
-    self.class.base_dir(@model)
+  # enforce the usage of Hashed storage when storing to
+  # remote store as the FileMover doesn't support OS
+  def base_dir(store = nil)
+    self.class.base_dir(@model, store || object_store)
   end
 
   # we don't need to know the actual path, an uploader instance should be
@@ -86,15 +94,19 @@ class FileUploader < GitlabUploader
   end
 
   def upload_path
-    self.class.upload_path(dynamic_segment, identifier)
+    if file_storage?
+      # Legacy path relative to project.full_path
+      File.join(dynamic_segment, identifier)
+    else
+      File.join(store_dir, identifier)
     end
-
-  def model_path_segment
-    self.class.model_path_segment(@model)
   end
 
-  def store_dir
-    File.join(base_dir, dynamic_segment)
+  def store_dirs
+    {
+      Store::LOCAL => File.join(base_dir, dynamic_segment),
+      Store::REMOTE => File.join(base_dir(ObjectStorage::Store::REMOTE), dynamic_segment)
+    }
   end
 
   def markdown_link

app/uploaders/namespace_file_uploader.rb

@@ -4,7 +4,7 @@ class NamespaceFileUploader < FileUploader
     options.storage_path
   end
 
-  def self.base_dir(model)
+  def self.base_dir(model, _store = nil)
     File.join(options.base_dir, 'namespace', model_path_segment(model))
   end
 
@@ -20,7 +20,7 @@ class NamespaceFileUploader < FileUploader
   def store_dirs
     {
       Store::LOCAL => File.join(base_dir, dynamic_segment),
-      Store::REMOTE => File.join('namespace', model_path_segment, dynamic_segment)
+      Store::REMOTE => File.join('namespace', self.class.model_path_segment(model), dynamic_segment)
     }
   end
 end

app/uploaders/personal_file_uploader.rb

@@ -4,7 +4,7 @@ class PersonalFileUploader < FileUploader
     options.storage_path
   end
 
-  def self.base_dir(model)
+  def self.base_dir(model, _store = nil)
     File.join(options.base_dir, model_path_segment(model))
   end
 
@@ -34,7 +34,7 @@ class PersonalFileUploader < FileUploader
   def store_dirs
     {
       Store::LOCAL => File.join(base_dir, dynamic_segment),
-      Store::REMOTE => File.join(model_path_segment, dynamic_segment)
+      Store::REMOTE => File.join(self.class.model_path_segment(model), dynamic_segment)
     }
   end
 

ee/changelogs/unreleased/poc-upload-hashing-path.yml

+---
+title: File uploads in remote storage now support project renaming.
+merge_request: 4597
+author:
+type: fixed

spec/uploaders/file_uploader_spec.rb

@@ -11,20 +11,20 @@ describe FileUploader do
   shared_examples 'builds correct legacy storage paths' do
     include_examples 'builds correct paths',
                      store_dir: %r{awesome/project/\h+},
+                     upload_path: %r{\h+/<filename>},
                      absolute_path: %r{#{described_class.root}/awesome/project/secret/foo.jpg}
   end
 
-  shared_examples 'uses hashed storage' do
+  context 'legacy storage' do
+    it_behaves_like 'builds correct legacy storage paths'
+
+    context 'uses hashed storage' do
       context 'when rolled out attachments' do
         let(:project) { build_stubbed(:project, namespace: group, name: 'project') }
 
-      before do
-        allow(project).to receive(:disk_path).and_return('ca/fe/fe/ed')
-      end
-
-      it_behaves_like 'builds correct paths',
-                      store_dir: %r{ca/fe/fe/ed/\h+},
-                      absolute_path: %r{#{described_class.root}/ca/fe/fe/ed/secret/foo.jpg}
+        include_examples 'builds correct paths',
+                         store_dir: %r{@hashed/\h{2}/\h{2}/\h+},
+                         upload_path: %r{\h+/<filename>}
       end
 
       context 'when only repositories are rolled out' do
@@ -33,10 +33,6 @@ describe FileUploader do
         it_behaves_like 'builds correct legacy storage paths'
       end
     end
-
-  context 'legacy storage' do
-    it_behaves_like 'builds correct legacy storage paths'
-    include_examples 'uses hashed storage'
   end
 
   context 'object store is remote' do
@@ -46,8 +42,10 @@ describe FileUploader do
 
     include_context 'with storage', described_class::Store::REMOTE
 
-    it_behaves_like 'builds correct legacy storage paths'
-    include_examples 'uses hashed storage'
+    # always use hashed storage path for remote uploads
+    it_behaves_like 'builds correct paths',
+                     store_dir: %r{@hashed/\h{2}/\h{2}/\h+},
+                     upload_path: %r{@hashed/\h{2}/\h{2}/\h+/\h+/<filename>}
   end
 
   describe 'initialize' do