Fix tests and code to make it work properly

app/controllers/projects/lfs_storage_controller.rb

@@ -32,7 +32,7 @@ class Projects::LfsStorageController < Projects::GitHttpClientController
     end
   rescue ActiveRecord::RecordInvalid
     render_400
-  rescue ArgumentError
+  rescue ObjectStorage::RemoteStoreError
     render_lfs_forbidden
   end
 

config/gitlab.yml.example

@@ -791,7 +791,7 @@ test:
         provider: AWS # Only AWS supported at the moment
         aws_access_key_id: AWS_ACCESS_KEY_ID
         aws_secret_access_key: AWS_SECRET_ACCESS_KEY
-        region: eu-central-1
+        region: us-east-1
   artifacts:
     path: tmp/tests/artifacts
     enabled: true
@@ -805,7 +805,7 @@ test:
         provider: AWS # Only AWS supported at the moment
         aws_access_key_id: AWS_ACCESS_KEY_ID
         aws_secret_access_key: AWS_SECRET_ACCESS_KEY
-        region: eu-central-1
+        region: us-east-1
   uploads:
     storage_path: tmp/tests/public
     enabled: true
@@ -815,7 +815,7 @@ test:
         provider: AWS # Only AWS supported at the moment
         aws_access_key_id: AWS_ACCESS_KEY_ID
         aws_secret_access_key: AWS_SECRET_ACCESS_KEY
-        region: eu-central-1
+        region: us-east-1
   gitlab:
     host: localhost
     port: 80

config/initializers/carrierwave.rb

@@ -28,16 +28,4 @@ if File.exist?(aws_file)
     # when fog_public is false and provider is AWS or Google, defaults to 600
     config.fog_authenticated_url_expiration = 1 << 29
   end
-
-  # Mocking Fog requests, based on: https://github.com/carrierwaveuploader/carrierwave/wiki/How-to%3A-Test-Fog-based-uploaders
-  if Rails.env.test?
-    Fog.mock!
-    connection = ::Fog::Storage.new(
-      aws_access_key_id: AWS_CONFIG['access_key_id'],
-      aws_secret_access_key: AWS_CONFIG['secret_access_key'],
-      provider: 'AWS',
-      region: AWS_CONFIG['region']
-    )
-    connection.directories.create(key: AWS_CONFIG['bucket'])
-  end
 end

ee/app/uploaders/object_storage.rb

@@ -163,8 +163,6 @@ module ObjectStorage
         File.join(self.root, TMP_UPLOAD_PATH)
       end
 
-      private
-
       def workhorse_remote_upload_options
         return unless self.object_store_enabled?
         return unless self.direct_upload_enabled?
@@ -230,10 +228,6 @@ module ObjectStorage
       end
     end
 
-    def filename
-      super || file&.filename
-    end
-
     #
     # Move the file to another store
     #
@@ -302,7 +296,7 @@ module ObjectStorage
       elsif local_path = params["#{identifier}.path"]
         store_local_file!(local_path, filename)
       else
-        raise ArgumentError, 'Bad file'
+        raise RemoteStoreError, 'Bad file'
       end
     end
 
@@ -316,30 +310,44 @@ module ObjectStorage
     end
 
     def store_remote_file!(remote_object_id, filename)
+      raise RemoteStoreError, 'Missing filename' unless filename
+
       file_path = File.join(TMP_UPLOAD_PATH, remote_object_id)
-      raise ArgumentError, 'Bad file path' unless file_path.start_with?(TMP_UPLOAD_PATH + '/')
+      file_path = Pathname.new(file_path).cleanpath.to_s
+      raise RemoteStoreError, 'Bad file path' unless file_path.start_with?(TMP_UPLOAD_PATH + '/')
 
       self.object_store = Store::REMOTE
-      self.original_filename = filename
 
+      # TODO:
+      # This should be changed to make use of `tmp/cache` mechanism
+      # instead of using custom upload directory,
+      # using tmp/cache makes this implementation way easier than it is today
       CarrierWave::Storage::Fog::File.new(self, storage, file_path).tap do |file|
-        raise ArgumentError, 'Missing file' unless file.exists?
+        raise RemoteStoreError, 'Missing file' unless file.exists?
 
-        storage.store!(file)
+        self.filename = filename
+        self.file = storage.store!(file)
       end
     end
 
     def store_local_file!(local_path, filename)
+      raise RemoteStoreError, 'Missing filename' unless filename
+
       root_path = File.realpath(self.class.workhorse_local_upload_path)
       file_path = File.realpath(local_path)
-      raise ArgumentError, 'Bad file path' unless file_path.start_with?(root_path)
+      raise RemoteStoreError, 'Bad file path' unless file_path.start_with?(root_path)
 
       self.object_store = Store::LOCAL
-      self.original_filename = filename
+      self.store!(UploadedFile.new(file_path, filename))
+    end
 
-      File.open(local_path) do |file|
-        self.store!(file)
+    # allow to configure and overwrite the filename
+    def filename
+      @filename || super || file&.filename # rubocop:disable Gitlab/ModuleWithInstanceVariables
     end
+
+    def filename=(filename)
+      @filename = filename # rubocop:disable Gitlab/ModuleWithInstanceVariables
     end
 
     # this is a hack around CarrierWave. The #migrate method needs to be

ee/changelogs/unreleased/store-lfs-on-object-storage-by-default.yml

+---
+title: Add support for direct uploading of LFS artifacts
+merge_request:
+author:
+type: added

ee/spec/support/stub_object_storage.rb

@@ -3,8 +3,6 @@ module StubConfiguration
     config:, uploader:, remote_directory:, enabled: true, licensed: true,
     background_upload: false, direct_upload: false
   )
-    Fog.mock!
-
     allow(config).to receive(:enabled) { enabled }
     allow(config).to receive(:background_upload) { background_upload }
     allow(config).to receive(:direct_upload) { direct_upload }
@@ -13,6 +11,8 @@ module StubConfiguration
 
     return unless enabled
 
+    Fog.mock!
+
     ::Fog::Storage.new(uploader.object_store_credentials).tap do |connection|
       begin
         connection.directories.create(key: remote_directory)

spec/lib/backup/manager_spec.rb

@@ -278,6 +278,10 @@ describe Backup::Manager do
       connection.directories.create(key: Gitlab.config.backup.upload.remote_directory)
     end
 
+    after do
+      Fog.unmock!
+    end
+
     context 'target path' do
       it 'uses the tar filename by default' do
         expect_any_instance_of(Fog::Collection).to receive(:create)

spec/requests/lfs_http_spec.rb

@@ -1104,7 +1104,9 @@ describe 'Git LFS API and storage' do
                   end
 
                   subject do
-                    put_finalize_with_args('file.object_id' => '12312300')
+                    put_finalize_with_args(
+                      'file.object_id' => '12312300',
+                      'file.name' => 'name')
                   end
 
                   it 'responds with status 200' do
@@ -1364,7 +1366,8 @@ describe 'Git LFS API and storage' do
       end
 
       args = {
-        'file.path' => file_path
+        'file.path' => file_path,
+        'file.name' => File.basename(file_path)
       }.compact
 
       put_finalize_with_args(args)

spec/uploaders/gitlab_uploader_spec.rb

@@ -27,7 +27,7 @@ describe GitlabUploader do
   describe '#file_cache_storage?' do
     context 'when file storage is used' do
       before do
-        uploader_class.cache_storage(:file)
+        expect(uploader_class).to receive(:cache_storage) { CarrierWave::Storage::File }
       end
 
       it { is_expected.to be_file_cache_storage }
@@ -35,7 +35,7 @@ describe GitlabUploader do
 
     context 'when is remote storage' do
       before do
-        uploader_class.cache_storage(:fog)
+        expect(uploader_class).to receive(:cache_storage) { CarrierWave::Storage::Fog }
       end
 
       it { is_expected.not_to be_file_cache_storage }