Merge branch 'fj-39265-create-snippet-repository-content' into 'master'

Create snippet repository content

See merge request gitlab-org/gitlab!23953

app/models/snippet_repository.rb

@@ -3,11 +3,76 @@
 class SnippetRepository < ApplicationRecord
   include Shardable
 
+  DEFAULT_EMPTY_FILE_NAME = 'snippetfile'
+  EMPTY_FILE_PATTERN = /^#{DEFAULT_EMPTY_FILE_NAME}(\d)\.txt$/.freeze
+
+  CommitError = Class.new(StandardError)
+
   belongs_to :snippet, inverse_of: :snippet_repository
 
+  delegate :repository, to: :snippet
+
   class << self
     def find_snippet(disk_path)
       find_by(disk_path: disk_path)&.snippet
     end
   end
+
+  def multi_files_action(user, files = [], **options)
+    return if files.nil? || files.empty?
+
+    lease_key = "multi_files_action:#{snippet_id}"
+
+    lease = Gitlab::ExclusiveLease.new(lease_key, timeout: 120)
+    raise CommitError, 'Snippet is already being updated' unless uuid = lease.try_obtain
+
+    options[:actions] = transform_file_entries(files)
+
+    capture_git_error { repository.multi_action(user, **options) }
+  ensure
+    Gitlab::ExclusiveLease.cancel(lease_key, uuid)
+  end
+
+  private
+
+  def capture_git_error(&block)
+    yield block
+  rescue Gitlab::Git::Index::IndexError,
+         Gitlab::Git::CommitError,
+         Gitlab::Git::PreReceiveError,
+         Gitlab::Git::CommandError => e
+    raise CommitError, e.message
+  end
+
+  def transform_file_entries(files)
+    last_index = get_last_empty_file_index
+
+    files.each do |file_entry|
+      file_entry[:action] = infer_action(file_entry) unless file_entry[:action]
+
+      if file_entry[:file_path].blank?
+        file_entry[:file_path] = build_empty_file_name(last_index)
+        last_index += 1
+      end
+    end
+  end
+
+  def infer_action(file_entry)
+    return :create if file_entry[:previous_path].blank?
+
+    file_entry[:previous_path] != file_entry[:file_path] ? :move : :update
+  end
+
+  def get_last_empty_file_index
+    last_file = repository.ls_files(nil)
+                          .map! { |file| file.match(EMPTY_FILE_PATTERN) }
+                          .compact
+                          .max_by { |element| element[1] }
+
+    last_file ? (last_file[1].to_i + 1) : 1
+  end
+
+  def build_empty_file_name(index)
+    "#{DEFAULT_EMPTY_FILE_NAME}#{index}.txt"
+  end
 end

app/services/snippets/create_service.rb

@@ -4,6 +4,8 @@ module Snippets
   class CreateService < Snippets::BaseService
     include SpamCheckMethods
 
+    CreateRepositoryError = Class.new(StandardError)
+
     def execute
       filter_spam_check_params
 
@@ -23,13 +25,7 @@ module Snippets
 
       spam_check(snippet, current_user)
 
-      snippet_saved = snippet.with_transaction_returning_status do
-        (snippet.save && snippet.store_mentions!).tap do |saved|
-          create_repository_for(snippet, current_user) if saved
-        end
-      end
-
-      if snippet_saved
+      if save_and_commit(snippet)
         UserAgentDetailService.new(snippet, @request).create
         Gitlab::UsageDataCounters::SnippetCounter.count(:create)
 
@@ -41,8 +37,45 @@ module Snippets
 
     private
 
-    def create_repository_for(snippet, user)
-      snippet.create_repository if Feature.enabled?(:version_snippets, user)
+    def save_and_commit(snippet)
+      snippet.with_transaction_returning_status do
+        (snippet.save && snippet.store_mentions!).tap do |saved|
+          break false unless saved
+
+          if Feature.enabled?(:version_snippets, current_user)
+            create_repository_for(snippet)
+            create_commit(snippet)
+          end
+        end
+      rescue => e # Rescuing all because we can receive Creation exceptions, GRPC exceptions, Git exceptions, ...
+        snippet.errors.add(:base, e.message)
+
+        # If the commit action failed we need to remove the repository if exists
+        if snippet.repository_exists?
+          Repositories::DestroyService.new(snippet.repository).execute
+        end
+
+        false
+      end
+    end
+
+    def create_repository_for(snippet)
+      snippet.create_repository
+
+      raise CreateRepositoryError, 'Repository could not be created' unless snippet.repository_exists?
+    end
+
+    def create_commit(snippet)
+      commit_attrs = {
+        branch_name: 'master',
+        message: 'Initial commit'
+      }
+
+      snippet.snippet_repository.multi_files_action(current_user, snippet_files, commit_attrs)
+    end
+
+    def snippet_files
+      [{ file_path: params[:file_name], content: params[:content] }]
     end
   end
 end

changelogs/unreleased/fj-39265-create-snippet-repository-content.yml

+---
+title: Commit file when snippet is created
+merge_request: 23953
+author:
+type: added

spec/models/snippet_repository_spec.rb

@@ -3,6 +3,11 @@
 require 'spec_helper'
 
 describe SnippetRepository do
+  let_it_be(:user) { create(:user) }
+  let(:snippet) { create(:personal_snippet, :repository, author: user) }
+  let(:snippet_repository) { snippet.snippet_repository }
+  let(:commit_opts) { { branch_name: 'master', message: 'whatever' } }
+
   describe 'associations' do
     it { is_expected.to belong_to(:shard) }
     it { is_expected.to belong_to(:snippet) }
@@ -10,7 +15,7 @@ describe SnippetRepository do
 
   describe '.find_snippet' do
     it 'finds snippet by disk path' do
-      snippet = create(:snippet)
+      snippet = create(:snippet, author: user)
       snippet.track_snippet_repository
 
       expect(described_class.find_snippet(snippet.disk_path)).to eq(snippet)
@@ -20,4 +25,147 @@ describe SnippetRepository do
       expect(described_class.find_snippet('@@unexisting/path/to/snippet')).to be_nil
     end
   end
+
+  describe '#multi_files_action' do
+    let(:new_file) { { file_path: 'new_file_test', content: 'bar' } }
+    let(:move_file) { { previous_path: 'CHANGELOG', file_path: 'CHANGELOG_new', content: 'bar' } }
+    let(:update_file) { { previous_path: 'README', file_path: 'README', content: 'bar' } }
+    let(:data) { [new_file, move_file, update_file] }
+
+    it 'returns nil when files argument is empty' do
+      expect(snippet.repository).not_to receive(:multi_action)
+
+      operation = snippet_repository.multi_files_action(user, [], commit_opts)
+
+      expect(operation).to be_nil
+    end
+
+    it 'returns nil when files argument is nil' do
+      expect(snippet.repository).not_to receive(:multi_action)
+
+      operation = snippet_repository.multi_files_action(user, nil, commit_opts)
+
+      expect(operation).to be_nil
+    end
+
+    it 'performs the operation accordingly to the files data' do
+      new_file_blob = blob_at(snippet, new_file[:file_path])
+      move_file_blob = blob_at(snippet, move_file[:previous_path])
+      update_file_blob = blob_at(snippet, update_file[:previous_path])
+
+      aggregate_failures do
+        expect(new_file_blob).to be_nil
+        expect(move_file_blob).not_to be_nil
+        expect(update_file_blob).not_to be_nil
+      end
+
+      expect do
+        snippet_repository.multi_files_action(user, data, commit_opts)
+      end.not_to raise_error
+
+      aggregate_failures do
+        data.each do |entry|
+          blob = blob_at(snippet, entry[:file_path])
+
+          expect(blob).not_to be_nil
+          expect(blob.path).to eq entry[:file_path]
+          expect(blob.data).to eq entry[:content]
+        end
+      end
+    end
+
+    it 'tries to obtain an exclusive lease' do
+      expect(Gitlab::ExclusiveLease).to receive(:new).with("multi_files_action:#{snippet.id}", anything).and_call_original
+
+      snippet_repository.multi_files_action(user, data, commit_opts)
+    end
+
+    it 'cancels the lease when the method has finished' do
+      expect(Gitlab::ExclusiveLease).to receive(:cancel).with("multi_files_action:#{snippet.id}", anything).and_call_original
+
+      snippet_repository.multi_files_action(user, data, commit_opts)
+    end
+
+    it 'raises an error if the lease cannot be obtained' do
+      allow_next_instance_of(Gitlab::ExclusiveLease) do |instance|
+        allow(instance).to receive(:try_obtain).and_return false
+      end
+
+      expect do
+        snippet_repository.multi_files_action(user, data, commit_opts)
+      end.to raise_error(described_class::CommitError)
+    end
+
+    context 'with commit actions' do
+      let(:result) do
+        [{ action: :create }.merge(new_file),
+         { action: :move }.merge(move_file),
+         { action: :update }.merge(update_file)]
+      end
+      let(:repo) { double }
+
+      before do
+        allow(snippet).to receive(:repository).and_return(repo)
+        allow(repo).to receive(:ls_files).and_return([])
+      end
+
+      it 'infers the commit action based on the parameters if not present' do
+        expect(repo).to receive(:multi_action).with(user, hash_including(actions: result))
+
+        snippet_repository.multi_files_action(user, data, commit_opts)
+      end
+
+      context 'when commit actions are present' do
+        let(:file_action) { { file_path: 'foo.txt', content: 'foo', action: :foobar } }
+        let(:data) { [file_action] }
+
+        it 'does not change commit action' do
+          expect(repo).to(
+            receive(:multi_action).with(
+              user,
+              hash_including(actions: array_including(hash_including(action: :foobar)))))
+
+          snippet_repository.multi_files_action(user, data, commit_opts)
+        end
+      end
+    end
+
+    context 'when files are not named' do
+      let(:data) do
+        [
+          {
+            file_path: '',
+            content: 'foo',
+            action: :create
+          },
+          {
+            file_path: '',
+            content: 'bar',
+            action: :create
+          },
+          {
+            file_path: 'foo.txt',
+            content: 'bar',
+            action: :create
+          }
+        ]
+      end
+
+      it 'sets a name for non named files' do
+        expect do
+          snippet_repository.multi_files_action(user, data, commit_opts)
+        end.not_to raise_error
+
+        expect(snippet.repository.ls_files(nil)).to include('snippetfile1.txt', 'snippetfile2.txt', 'foo.txt')
+      end
+    end
+  end
+
+  def blob_at(snippet, path)
+    snippet.repository.blob_at('master', path)
+  end
+
+  def first_blob(snippet)
+    snippet.repository.blob_at('master', snippet.repository.ls_files(nil).first)
+  end
 end

spec/requests/api/project_snippets_spec.rb

@@ -98,6 +98,36 @@ describe API::ProjectSnippets do
       }
     end
 
+    shared_examples 'project snippet repository actions' do
+      let(:snippet) { ProjectSnippet.find(json_response['id']) }
+
+      it 'creates repository' do
+        subject
+
+        expect(snippet.repository.exists?).to be_truthy
+      end
+
+      it 'commit the files to the repository' do
+        subject
+
+        blob = snippet.repository.blob_at('master', params[:file_name])
+
+        expect(blob.data).to eq params[:code]
+      end
+
+      context 'when feature flag :version_snippets is disabled' do
+        it 'does not create snippet repository' do
+          stub_feature_flags(version_snippets: false)
+
+          expect do
+            subject
+          end.to change { ProjectSnippet.count }.by(1)
+
+          expect(snippet.repository_exists?).to be_falsey
+        end
+      end
+    end
+
     context 'with a regular user' do
       let(:user) { create(:user) }
 
@@ -118,6 +148,10 @@ describe API::ProjectSnippets do
         expect(snippet.file_name).to eq(params[:file_name])
         expect(snippet.visibility_level).to eq(Snippet::INTERNAL)
       end
+
+      it_behaves_like 'project snippet repository actions' do
+        subject { post api("/projects/#{project.id}/snippets/", user), params: params }
+      end
     end
 
     it 'creates a new snippet' do
@@ -132,6 +166,10 @@ describe API::ProjectSnippets do
       expect(snippet.visibility_level).to eq(Snippet::PUBLIC)
     end
 
+    it_behaves_like 'project snippet repository actions' do
+      subject { post api("/projects/#{project.id}/snippets/", admin), params: params }
+    end
+
     it 'creates a new snippet with content parameter' do
       params[:content] = params.delete(:code)
 

spec/requests/api/snippets_spec.rb

@@ -199,9 +199,13 @@ describe API::Snippets do
     end
 
     shared_examples 'snippet creation' do
+      let(:snippet) { Snippet.find(json_response["id"]) }
+
+      subject { post api("/snippets/", user), params: params }
+
       it 'creates a new snippet' do
         expect do
-          post api("/snippets/", user), params: params
+          subject
         end.to change { PersonalSnippet.count }.by(1)
 
         expect(response).to have_gitlab_http_status(201)
@@ -210,6 +214,32 @@ describe API::Snippets do
         expect(json_response['file_name']).to eq(params[:file_name])
         expect(json_response['visibility']).to eq(params[:visibility])
       end
+
+      it 'creates repository' do
+        subject
+
+        expect(snippet.repository.exists?).to be_truthy
+      end
+
+      it 'commit the files to the repository' do
+        subject
+
+        blob = snippet.repository.blob_at('master', params[:file_name])
+
+        expect(blob.data).to eq params[:content]
+      end
+
+      context 'when feature flag :version_snippets is disabled' do
+        it 'does not create snippet repository' do
+          stub_feature_flags(version_snippets: false)
+
+          expect do
+            subject
+          end.to change { PersonalSnippet.count }.by(1)
+
+          expect(snippet.repository_exists?).to be_falsey
+        end
+      end
     end
 
     context 'with restricted visibility settings' do

spec/services/snippets/create_service_spec.rb

@@ -143,37 +143,102 @@ describe Snippets::CreateService do
       end
     end
 
-    shared_examples 'creates repository' do
-      it do
+    shared_examples 'creates repository and files' do
+      it 'creates repository' do
         subject
 
-        expect(snippet.repository_exists?).to be_truthy
+        expect(snippet.repository.exists?).to be_truthy
+      end
+
+      it 'commit the files to the repository' do
+        subject
+
+        blob = snippet.repository.blob_at('master', base_opts[:file_name])
+
+        expect(blob.data).to eq base_opts[:content]
+      end
+
+      context 'when repository creation action fails' do
+        before do
+          allow_next_instance_of(Snippet) do |instance|
+            allow(instance).to receive(:create_repository).and_return(nil)
+          end
+        end
+
+        it 'does not create the snippet' do
+          expect { subject }.not_to change { Snippet.count }
+        end
+
+        it 'returns the error' do
+          expect(snippet.errors.full_messages).to include('Repository could not be created')
+        end
+      end
+
+      context 'when the commit action fails' do
+        before do
+          allow_next_instance_of(SnippetRepository) do |instance|
+            allow(instance).to receive(:multi_files_action).and_raise(SnippetRepository::CommitError.new('foobar'))
+          end
+        end
+
+        it 'does not create the snippet' do
+          expect { subject }.not_to change { Snippet.count }
+        end
+
+        it 'does not create the repository' do
+          expect(snippet.repository_exists?).to be_falsey
+        end
+
+        it 'destroys the existing repository' do
+          expect(Repositories::DestroyService).to receive(:new).and_call_original
+
+          subject
+        end
+
+        it 'returns the error' do
+          response = subject
+
+          expect(response).to be_error
+          expect(response.payload[:snippet].errors.full_messages).to eq ['foobar']
+        end
       end
 
       context 'when snippet creation fails' do
         let(:extra_opts) { { content: nil } }
 
         it 'does not create repository' do
+          expect do
             subject
+          end.not_to change(Snippet, :count)
 
           expect(snippet.repository_exists?).to be_falsey
         end
       end
 
       context 'when feature flag :version_snippets is disabled' do
-        it 'does not create snippet repository' do
+        before do
           stub_feature_flags(version_snippets: false)
+        end
 
+        it 'does not create snippet repository' do
           expect do
             subject
           end.to change(Snippet, :count).by(1)
 
           expect(snippet.repository_exists?).to be_falsey
         end
+
+        it 'does not try to commit files' do
+          expect_next_instance_of(described_class) do |instance|
+            expect(instance).not_to receive(:create_commit)
+          end
+
+          subject
+        end
       end
     end
 
-    context 'when Project Snippet' do
+    context 'when ProjectSnippet' do
       let_it_be(:project) { create(:project) }
 
       before do
@@ -185,7 +250,7 @@ describe Snippets::CreateService do
       it_behaves_like 'spam check is performed'
       it_behaves_like 'snippet create data is tracked'
       it_behaves_like 'an error service response when save fails'
-      it_behaves_like 'creates repository'
+      it_behaves_like 'creates repository and files'
     end
 
     context 'when PersonalSnippet' do
@@ -196,7 +261,7 @@ describe Snippets::CreateService do
       it_behaves_like 'spam check is performed'
       it_behaves_like 'snippet create data is tracked'
       it_behaves_like 'an error service response when save fails'
-      it_behaves_like 'creates repository'
+      it_behaves_like 'creates repository and files'
     end
   end
 end