Multi-file upload and Commit API obey LFS filters

Updates Files::MultiService for the commits API which is in turn used by the
multi-file upload web UI

Ensures that files which should be in LFS are transformed into LFS pointers
Uses Lfs::Transformer which then links LfsObjectProjects on success

app/services/files/create_service.rb

@@ -1,9 +1,8 @@
 module Files
   class CreateService < Files::BaseService
     def create_commit!
-      handler = Lfs::FileTransformer.new(project, @branch_name)
-
-      handler.new_file(@file_path, @file_content) do |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
     end

app/services/files/multi_service.rb

@@ -3,11 +3,32 @@ 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])
+
+        commit_actions!(actions)
+      end
+    end
+
+    private
+
+    def actions_after_lfs_transformation(transformer, actions)
+      actions.map do |action|
+        if action[:action] == 'create'
+          content = transformer.new_file(action[:file_path], action[:content])
+          action[:content] = content
+        end
+
+        action
+      end
+    end
+
+    def commit_actions!(actions)
       repository.multi_action(
         current_user,
         message: @commit_message,
         branch_name: @branch_name,
-        actions: params[:actions],
+        actions: actions,
         author_email: @author_email,
         author_name: @author_name,
         start_project: @start_project,
@@ -17,8 +38,6 @@ module Files
       raise_error(e)
     end
 
-    private
-
     def validate!
       super
 

app/services/lfs/file_transformer.rb

@@ -1,4 +1,16 @@
 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.
+  #
+  #        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.
+  #
+  # 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
 
@@ -9,26 +21,41 @@ 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)
-        content = lfs_pointer_file.pointer
 
-        success = yield(content)
+        on_success_actions << -> { link_lfs_object!(lfs_object) }
 
-        link_lfs_object!(lfs_object) if success
+        lfs_pointer_file.pointer
       else
-        yield(file_content)
+        file_content
       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)

changelogs/unreleased/jej-commit-api-tracks-lfs.yml

@@ -0,0 +1,5 @@
+---
+title: Create commit API and Web IDE obey LFS filters
+merge_request: 16718
+author:
+type: fixed

spec/services/files/multi_service_spec.rb

@@ -4,10 +4,12 @@ describe Files::MultiService do
   subject { described_class.new(project, user, commit_params) }
 
   let(:project) { create(:project, :repository) }
+  let(:repository) { project.repository }
   let(:user) { create(:user) }
   let(:branch_name) { project.default_branch }
   let(:original_file_path) { 'files/ruby/popen.rb' }
   let(:new_file_path) { 'files/ruby/popen.rb' }
+  let(:file_content) { 'New content' }
   let(:action) { 'update' }
 
   let!(:original_commit_id) do
@@ -20,7 +22,7 @@ describe Files::MultiService do
         action: action,
         file_path: new_file_path,
         previous_path: original_file_path,
-        content: 'New content',
+        content: file_content,
         last_commit_id: original_commit_id
       }
     ]
@@ -110,6 +112,36 @@ describe Files::MultiService do
       end
     end
 
+    context 'when creating a file matching an LFS filter' do
+      let(:action) { 'create' }
+      let(:branch_name) { 'lfs' }
+      let(:new_file_path) { 'test_file.lfs' }
+
+      before do
+        allow(project).to receive(:lfs_enabled?).and_return(true)
+      end
+
+      it 'creates an LFS pointer' do
+        subject.execute
+
+        blob = repository.blob_at('lfs', new_file_path)
+
+        expect(blob.data).to start_with('version https://git-lfs.github.com/spec/v1')
+      end
+
+      it "creates an LfsObject with the file's content" do
+        subject.execute
+
+        expect(LfsObject.last.file.read).to eq file_content
+      end
+
+      it 'links the LfsObject to the project' do
+        expect do
+          subject.execute
+        end.to change { project.lfs_objects.count }.by(1)
+      end
+    end
+
     context 'when file status validation is skipped' do
       let(:action) { 'create' }
       let(:new_file_path) { 'files/ruby/new_file.rb' }

spec/services/lfs/file_transformer_spec.rb

@@ -0,0 +1,100 @@
+require "spec_helper"
+
+describe Lfs::FileTransformer do
+  let(:project) { create(:project, :repository) }
+  let(:repository) { project.repository }
+  let(:file_content) { 'Test file content' }
+  let(:branch_name) { 'lfs' }
+  let(:file_path) { 'test_file.lfs' }
+
+  subject { described_class.new(project, branch_name) }
+
+  describe '#new_file' do
+    context 'with lfs disabled' do
+      it 'skips gitattributes check' do
+        expect(repository.raw).not_to receive(:blob_at)
+
+        subject.new_file(file_path, file_content)
+      end
+    end
+
+    context 'with lfs enabled' do
+      before do
+        allow(project).to receive(:lfs_enabled?).and_return(true)
+      end
+
+      it 'reuses cached gitattributes' do
+        second_file = 'another_file.lfs'
+
+        expect(repository.raw).to receive(:blob_at).with(branch_name, '.gitattributes').once
+
+        subject.new_file(file_path, file_content)
+        subject.new_file(second_file, file_content)
+      end
+
+      it "creates an LfsObject with the file's content" do
+        subject.new_file(file_path, file_content)
+
+        expect(LfsObject.last.file.read).to eq file_content
+      end
+
+      it 'creates an LFS pointer' do
+        new_content = subject.new_file(file_path, file_content)
+
+        expect(new_content).to start_with('version https://git-lfs.github.com/spec/v1')
+      end
+
+      context "when doesn't use LFS" do
+        let(:file_path) { 'other.filetype' }
+
+        it "doesn't create LFS pointers" do
+          new_content = subject.new_file(file_path, file_content)
+
+          expect(new_content).not_to start_with('version https://git-lfs.github.com/spec/v1')
+          expect(new_content).to eq(file_content)
+        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)
+      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 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)
+        end
+
+        it 'skips linking LfsObjects if the block returns falsey' 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
+        end
+      end
+    end
+  end
+end