Merge branch 'jej/commit-api-tracks-lfs' into 'master'

Create commit API and Web IDE obey LFS filters

Closes #42287

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

10 files changed, 270 insertions, 64 deletions

diff --git a/app/services/files/create_service.rb b/app/services/files/create_service.rb
index 46acdc5406c..a954564946b 100644
--- a/app/services/files/create_service.rb
+++ b/app/services/files/create_service.rb
@@ -1,11 +1,11 @@
 module Files
   class CreateService < Files::BaseService
     def create_commit!
-      handler = Lfs::FileModificationHandler.new(project, @branch_name)
+      transformer = Lfs::FileTransformer.new(project, @branch_name)
 
-      handler.new_file(@file_path, @file_content) do |content_or_lfs_pointer|
-        create_transformed_commit(content_or_lfs_pointer)
-      end
+      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 a03c59f569d..13a1dee4173 100644
--- a/app/services/files/multi_service.rb
+++ b/app/services/files/multi_service.rb
@@ -3,11 +3,33 @@ module Files
     UPDATE_FILE_ACTIONS = %w(update move delete).freeze
 
     def create_commit!
+      transformer = Lfs::FileTransformer.new(project, @branch_name)
+
+      actions = actions_after_lfs_transformation(transformer, params[:actions])
+
+      commit_actions!(actions)
+    end
+
+    private
+
+    def actions_after_lfs_transformation(transformer, actions)
+      actions.map do |action|
+        if action[:action] == 'create'
+          result = transformer.new_file(action[:file_path], action[:content], encoding: action[:encoding])
+          action[:content] = result.content
+          action[:encoding] = result.encoding
+        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 +39,6 @@ module Files
       raise_error(e)
     end
 
-    private
-
     def validate!
       super
 
diff --git a/app/services/lfs/file_modification_handler.rb b/app/services/lfs/file_modification_handler.rb
deleted file mode 100644
index fe9091a6e5d..00000000000
--- a/app/services/lfs/file_modification_handler.rb
+++ /dev/null
@@ -1,42 +0,0 @@
-module Lfs
-  class FileModificationHandler
-    attr_reader :project, :branch_name
-
-    delegate :repository, to: :project
-
-    def initialize(project, branch_name)
-      @project = project
-      @branch_name = branch_name
-    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)
-
-        link_lfs_object!(lfs_object) if success
-      else
-        yield(file_content)
-      end
-    end
-
-    private
-
-    def lfs_file?(file_path)
-      repository.attributes_at(branch_name, file_path)['filter'] == 'lfs'
-    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)
-      end
-    end
-
-    def link_lfs_object!(lfs_object)
-      project.lfs_objects << lfs_object
-    end
-  end
-end
diff --git a/app/services/lfs/file_transformer.rb b/app/services/lfs/file_transformer.rb
new file mode 100644
index 00000000000..69281ee3137
--- /dev/null
+++ b/app/services/lfs/file_transformer.rb
@@ -0,0 +1,66 @@
+module Lfs
+  # 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.
+  #
+  #        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)
+  #
+  class FileTransformer
+    attr_reader :project, :branch_name
+
+    delegate :repository, to: :project
+
+    def initialize(project, branch_name)
+      @project = project
+      @branch_name = branch_name
+    end
+
+    def new_file(file_path, file_content, encoding: nil)
+      if project.lfs_enabled? && lfs_file?(file_path)
+        file_content = Base64.decode64(file_content) if encoding == 'base64'
+        lfs_pointer_file = Gitlab::Git::LfsPointerFile.new(file_content)
+        lfs_object = create_lfs_object!(lfs_pointer_file, file_content)
+
+        link_lfs_object!(lfs_object)
+
+        Result.new(content: lfs_pointer_file.pointer, encoding: 'text')
+      else
+        Result.new(content: file_content, encoding: encoding)
+      end
+    end
+
+    class Result
+      attr_reader :content, :encoding
+
+      def initialize(content:, encoding:)
+        @content = content
+        @encoding = encoding
+      end
+    end
+
+    private
+
+    def lfs_file?(file_path)
+      cached_attributes.attributes(file_path)['filter'] == 'lfs'
+    end
+
+    def cached_attributes
+      @cached_attributes ||= Gitlab::Git::AttributesAtRefParser.new(repository, branch_name)
+    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)
+      end
+    end
+
+    def link_lfs_object!(lfs_object)
+      project.lfs_objects << lfs_object
+    end
+  end
+end
diff --git a/changelogs/unreleased/jej-commit-api-tracks-lfs.yml b/changelogs/unreleased/jej-commit-api-tracks-lfs.yml
new file mode 100644
index 00000000000..8284abf9f28
--- /dev/null
+++ b/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
diff --git a/lib/gitlab/git/lfs_pointer_file.rb b/lib/gitlab/git/lfs_pointer_file.rb
index da12ed7d125..2ae0a889590 100644
--- a/lib/gitlab/git/lfs_pointer_file.rb
+++ b/lib/gitlab/git/lfs_pointer_file.rb
@@ -1,13 +1,16 @@
 module Gitlab
   module Git
     class LfsPointerFile
+      VERSION = "https://git-lfs.github.com/spec/v1".freeze
+      VERSION_LINE = "version #{VERSION}".freeze
+
       def initialize(data)
         @data = data
       end
 
       def pointer
         @pointer ||= <<~FILE
-          version https://git-lfs.github.com/spec/v1
+          #{VERSION_LINE}
           oid sha256:#{sha256}
           size #{size}
         FILE
@@ -20,6 +23,10 @@ module Gitlab
       def sha256
         @sha256 ||= Digest::SHA256.hexdigest(@data)
       end
+
+      def inspect
+        "#<#{self.class}:#{object_id} @size=#{size}, @sha256=#{sha256.inspect}>"
+      end
     end
   end
 end
diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb
index fbc93542619..9811c447a01 100644
--- a/lib/gitlab/git/repository.rb
+++ b/lib/gitlab/git/repository.rb
@@ -1002,8 +1002,9 @@ module Gitlab
       # This only checks the root .gitattributes file,
       # it does not traverse subfolders to find additional .gitattributes files
       #
-      # This method is around 30 times slower than `attributes`,
-      # which uses `$GIT_DIR/info/attributes`
+      # This method is around 30 times slower than `attributes`, which uses
+      # `$GIT_DIR/info/attributes`. Consider caching AttributesAtRefParser
+      # and reusing that for multiple calls instead of this method.
       def attributes_at(ref, file_path)
         parser = AttributesAtRefParser.new(self, ref)
         parser.attributes(file_path)
diff --git a/spec/services/files/create_service_spec.rb b/spec/services/files/create_service_spec.rb
index 030263b1502..abe99b9e794 100644
--- a/spec/services/files/create_service_spec.rb
+++ b/spec/services/files/create_service_spec.rb
@@ -43,7 +43,7 @@ describe Files::CreateService do
 
           blob = repository.blob_at('lfs', file_path)
 
-          expect(blob.data).not_to start_with('version https://git-lfs.github.com/spec/v1')
+          expect(blob.data).not_to start_with(Gitlab::Git::LfsPointerFile::VERSION_LINE)
           expect(blob.data).to eq(file_content)
         end
       end
@@ -58,7 +58,7 @@ describe Files::CreateService do
 
           blob = repository.blob_at('lfs', file_path)
 
-          expect(blob.data).to start_with('version https://git-lfs.github.com/spec/v1')
+          expect(blob.data).to start_with(Gitlab::Git::LfsPointerFile::VERSION_LINE)
         end
 
         it "creates an LfsObject with the file's content" do
diff --git a/spec/services/files/multi_service_spec.rb b/spec/services/files/multi_service_spec.rb
index b9971776b33..59984c10990 100644
--- a/spec/services/files/multi_service_spec.rb
+++ b/spec/services/files/multi_service_spec.rb
@@ -4,28 +4,30 @@ 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
     Gitlab::Git::Commit.last_for_path(project.repository, branch_name, original_file_path).sha
   end
 
-  let(:actions) do
-    [
-      {
-        action: action,
-        file_path: new_file_path,
-        previous_path: original_file_path,
-        content: 'New content',
-        last_commit_id: original_commit_id
-      }
-    ]
+  let(:default_action) do
+    {
+      action: action,
+      file_path: new_file_path,
+      previous_path: original_file_path,
+      content: file_content,
+      last_commit_id: original_commit_id
+    }
   end
 
+  let(:actions) { [default_action] }
+
   let(:commit_params) do
     {
       commit_message: "Update File",
@@ -110,6 +112,56 @@ 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(Gitlab::Git::LfsPointerFile::VERSION_LINE)
+      end
+
+      it "creates an LfsObject with the file's content" do
+        subject.execute
+
+        expect(LfsObject.last.file.read).to eq file_content
+      end
+
+      context 'with base64 encoded content' do
+        let(:raw_file_content) { 'Raw content' }
+        let(:file_content) { Base64.encode64(raw_file_content) }
+        let(:actions) { [default_action.merge(encoding: 'base64')] }
+
+        it 'creates an LFS pointer' do
+          subject.execute
+
+          blob = repository.blob_at('lfs', new_file_path)
+
+          expect(blob.data).to start_with(Gitlab::Git::LfsPointerFile::VERSION_LINE)
+        end
+
+        it "creates an LfsObject with the file's content" do
+          subject.execute
+
+          expect(LfsObject.last.file.read).to eq raw_file_content
+        end
+      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' }
diff --git a/spec/services/lfs/file_transformer_spec.rb b/spec/services/lfs/file_transformer_spec.rb
new file mode 100644
index 00000000000..e8938338cb7
--- /dev/null
+++ b/spec/services/lfs/file_transformer_spec.rb
@@ -0,0 +1,97 @@
+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
+
+      it 'returns untransformed content' do
+        result = subject.new_file(file_path, file_content)
+
+        expect(result.content).to eq(file_content)
+      end
+
+      it 'returns untransformed encoding' do
+        result = subject.new_file(file_path, file_content, encoding: 'base64')
+
+        expect(result.encoding).to eq('base64')
+      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 'returns an LFS pointer' do
+        result = subject.new_file(file_path, file_content)
+
+        expect(result.content).to start_with(Gitlab::Git::LfsPointerFile::VERSION_LINE)
+      end
+
+      it 'returns LFS pointer encoding as text' do
+        result = subject.new_file(file_path, file_content, encoding: 'base64')
+
+        expect(result.encoding).to eq('text')
+      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).content
+
+          expect(new_content).not_to start_with(Gitlab::Git::LfsPointerFile::VERSION_LINE)
+          expect(new_content).to eq(file_content)
+        end
+      end
+
+      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
+
+      context 'when LfsObject already exists' do
+        let(:lfs_pointer) { Gitlab::Git::LfsPointerFile.new(file_content) }
+
+        before do
+          create(:lfs_object, oid: lfs_pointer.sha256, size: lfs_pointer.size)
+        end
+
+        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
+  end
+end