Avoid failed integrity check by linking LfsObjectProjects sooner

Attempted commits were failing due to the pre-recieve LfsIntegrity
check. This is skipped during tests making this failure silent.

4 files changed, 30 insertions, 67 deletions

diff --git a/app/services/files/create_service.rb b/app/services/files/create_service.rb
index b8ae03842a3..a954564946b 100644
--- a/app/services/files/create_service.rb
+++ b/app/services/files/create_service.rb
@@ -1,10 +1,11 @@
 module Files
   class CreateService < Files::BaseService
     def create_commit!
-      Lfs::FileTransformer.link_lfs_objects(project, @branch_name) do |transformer|
-        content_or_lfs_pointer = transformer.new_file(@file_path, @file_content)
-        create_transformed_commit(content_or_lfs_pointer)
-      end
+      transformer = Lfs::FileTransformer.new(project, @branch_name)
+
+      result = transformer.new_file(@file_path, @file_content)
+
+      create_transformed_commit(result.content)
     end
 
     private
diff --git a/app/services/files/multi_service.rb b/app/services/files/multi_service.rb
index bd88c46e4af..be9c7c526a6 100644
--- a/app/services/files/multi_service.rb
+++ b/app/services/files/multi_service.rb
@@ -3,11 +3,11 @@ module Files
     UPDATE_FILE_ACTIONS = %w(update move delete).freeze
 
     def create_commit!
-      Lfs::FileTransformer.link_lfs_objects(project, @branch_name) do |transformer|
-        actions = actions_after_lfs_transformation(transformer, params[:actions])
+      transformer = Lfs::FileTransformer.new(project, @branch_name)
 
-        commit_actions!(actions)
-      end
+      actions = actions_after_lfs_transformation(transformer, params[:actions])
+
+      commit_actions!(actions)
     end
 
     private
diff --git a/app/services/lfs/file_transformer.rb b/app/services/lfs/file_transformer.rb
index 7c93659b60d..0235d07e529 100644
--- a/app/services/lfs/file_transformer.rb
+++ b/app/services/lfs/file_transformer.rb
@@ -1,16 +1,15 @@
 module Lfs
-  # Usage: Open a `.link_lfs_objects` block, call `new_file` on the yielded object
-  #        as many times as needed. LfsObjectProject links will be saved if the
-  #        return value of the block is truthy.
+  # Usage: Calling `new_file` check to see if a file should be in LFS and
+  #        return a transformed result with `content` and `encoding` to commit.
   #
-  #        Calling `new_file` will check the path to see if it should be in LFS,
-  #        save and LFS pointer of needed and return its content to be saved in
-  #        a commit. If the file isn't LFS the untransformed content is returned.
+  #        For LFS an LfsObject linked to the project is stored and an LFS
+  #        pointer returned. If the file isn't in LFS the untransformed content
+  #        is returned to save in the commit.
+  #
+  # transformer = Lfs::FileTransformer.new(project, @branch_name)
+  # content_or_lfs_pointer = transformer.new_file(file_path, content).content
+  # create_transformed_commit(content_or_lfs_pointer)
   #
-  # Lfs::FileTransformer.link_lfs_objects(project, @branch_name) do |transformer|
-  #   content_or_lfs_pointer = transformer.new_file(file_path, file_content)
-  #   create_transformed_commit(content_or_lfs_pointer)
-  # end
   class FileTransformer
     attr_reader :project, :branch_name
 
@@ -21,20 +20,12 @@ module Lfs
       @branch_name = branch_name
     end
 
-    def self.link_lfs_objects(project, branch_name)
-      transformer = new(project, branch_name)
-      result = yield(transformer)
-      transformer.after_transform! if result
-
-      result
-    end
-
     def new_file(file_path, file_content)
       if project.lfs_enabled? && lfs_file?(file_path)
         lfs_pointer_file = Gitlab::Git::LfsPointerFile.new(file_content)
         lfs_object = create_lfs_object!(lfs_pointer_file, file_content)
 
-        on_success_actions << -> { link_lfs_object!(lfs_object) }
+        link_lfs_object!(lfs_object)
 
         lfs_pointer_file.pointer
       else
@@ -42,20 +33,12 @@ module Lfs
       end
     end
 
-    def after_transform!
-      on_success_actions.map(&:call)
-    end
-
     private
 
     def lfs_file?(file_path)
       repository.attributes_at(branch_name, file_path)['filter'] == 'lfs'
     end
 
-    def on_success_actions
-      @on_success_actions ||= []
-    end
-
     def create_lfs_object!(lfs_pointer_file, file_content)
       LfsObject.find_or_create_by(oid: lfs_pointer_file.sha256, size: lfs_pointer_file.size) do |lfs_object|
         lfs_object.file = CarrierWaveStringFile.new(file_content)
diff --git a/spec/services/lfs/file_transformer_spec.rb b/spec/services/lfs/file_transformer_spec.rb
index f469f5e76dd..6d17ab03ac3 100644
--- a/spec/services/lfs/file_transformer_spec.rb
+++ b/spec/services/lfs/file_transformer_spec.rb
@@ -55,44 +55,23 @@ describe Lfs::FileTransformer do
         end
       end
 
-      it 'sets up after_transform! to link LfsObjects to project' do
-        subject.new_file(file_path, file_content)
-
-        expect { subject.after_transform! }.to change { project.lfs_objects.count }.by(1)
+      it 'links LfsObjects to project' do
+        expect do
+          subject.new_file(file_path, file_content)
+        end.to change { project.lfs_objects.count }.by(1)
       end
-    end
-  end
 
-  describe '.link_lfs_objects' do
-    context 'with lfs enabled' do
-      before do
-        allow(project).to receive(:lfs_enabled?).and_return(true)
-      end
+      context 'when LfsObject already exists' do
+        let(:lfs_pointer) { Gitlab::Git::LfsPointerFile.new(file_content) }
 
-      context 'when given a block' do
-        it 'links LfsObject to the project automatically' do
-          expect do
-            described_class.link_lfs_objects(project, branch_name) do |t|
-              t.new_file(file_path, file_content)
-            end
-          end.to change { project.lfs_objects.count }.by(1)
+        before do
+          create(:lfs_object, oid: lfs_pointer.sha256, size: lfs_pointer.size)
         end
 
-        it 'skips linking LfsObjects if the block returns falsey' do
+        it 'links LfsObjects to project' do
           expect do
-            described_class.link_lfs_objects(project, branch_name) do |t|
-              t.new_file(file_path, file_content)
-              false
-            end
-          end.not_to change { project.lfs_objects.count }
-        end
-
-        it 'returns the result of the block' do
-          result = described_class.link_lfs_objects(project, branch_name) do |t|
-            :dummy_commit
-          end
-
-          expect(result).to eq :dummy_commit
+            subject.new_file(file_path, file_content)
+          end.to change { project.lfs_objects.count }.by(1)
         end
       end
     end