Merge branch '239327-fj-fix-snippet-update-edge-case' into 'master'

Fix edge case when updating snippet with no repo

See merge request gitlab-org/gitlab!42964

app/services/snippets/base_service.rb

@@ -4,6 +4,9 @@ module Snippets
   class BaseService < ::BaseService
     include SpamCheckMethods
 
+    UPDATE_COMMIT_MSG = 'Update snippet'
+    INITIAL_COMMIT_MSG = 'Initial commit'
+
     CreateRepositoryError = Class.new(StandardError)
 
     attr_reader :uploaded_assets, :snippet_actions
@@ -85,5 +88,20 @@ module Snippets
     def restricted_files_actions
       nil
     end
+
+    def commit_attrs(snippet, msg)
+      {
+        branch_name: snippet.default_branch,
+        message: msg
+      }
+    end
+
+    def delete_repository(snippet)
+      snippet.repository.remove
+      snippet.snippet_repository&.delete
+
+      # Purge any existing value for repository_exists?
+      snippet.repository.expire_exists_cache
+    end
   end
 end

app/services/snippets/create_service.rb

@@ -59,7 +59,7 @@ module Snippets
       log_error(e.message)
 
       # If the commit action failed we need to remove the repository if exists
-      @snippet.repository.remove if @snippet.repository_exists?
+      delete_repository(@snippet) if @snippet.repository_exists?
 
       # If the snippet was created, we need to remove it as we
       # would do like if it had had any validation error
@@ -81,12 +81,9 @@ module Snippets
     end
 
     def create_commit
-      commit_attrs = {
-        branch_name: @snippet.default_branch,
-        message: 'Initial commit'
-      }
+      attrs = commit_attrs(@snippet, INITIAL_COMMIT_MSG)
 
-      @snippet.snippet_repository.multi_files_action(current_user, files_to_commit(@snippet), commit_attrs)
+      @snippet.snippet_repository.multi_files_action(current_user, files_to_commit(@snippet), attrs)
     end
 
     def move_temporary_files

app/services/snippets/update_service.rb

@@ -37,7 +37,10 @@ module Snippets
       # is implemented.
       # Once we can perform different operations through this service
       # we won't need to keep track of the `content` and `file_name` fields
-      if snippet_actions.any?
+      #
+      # If the repository does not exist we don't need to update `params`
+      # because we need to commit the information from the database
+      if snippet_actions.any? && snippet.repository_exists?
         params[:content] = snippet_actions[0].content if snippet_actions[0].content
         params[:file_name] = snippet_actions[0].file_path
       end
@@ -52,7 +55,11 @@ module Snippets
       # the repository we can just return
       return true unless committable_attributes?
 
-      create_repository_for(snippet)
+      unless snippet.repository_exists?
+        create_repository_for(snippet)
+        create_first_commit_using_db_data(snippet)
+      end
+
       create_commit(snippet)
 
       true
@@ -72,13 +79,7 @@ module Snippets
       # If the commit action failed we remove it because
       # we don't want to leave empty repositories
       # around, to allow cloning them.
-      if repository_empty?(snippet)
-        snippet.repository.remove
-        snippet.snippet_repository&.delete
-      end
-
-      # Purge any existing value for repository_exists?
-      snippet.repository.expire_exists_cache
+      delete_repository(snippet) if repository_empty?(snippet)
 
       false
     end
@@ -89,15 +90,25 @@ module Snippets
       raise CreateRepositoryError, 'Repository could not be created' unless snippet.repository_exists?
     end
 
+    # If the user provides `snippet_actions` and the repository
+    # does not exist, we need to commit first the snippet info stored
+    # in the database.  Mostly because the content inside `snippet_actions`
+    # would assume that the file is already in the repository.
+    def create_first_commit_using_db_data(snippet)
+      return if snippet_actions.empty?
+
+      attrs = commit_attrs(snippet, INITIAL_COMMIT_MSG)
+      actions = [{ file_path: snippet.file_name, content: snippet.content }]
+
+      snippet.snippet_repository.multi_files_action(current_user, actions, attrs)
+    end
+
     def create_commit(snippet)
       raise UpdateError unless snippet.snippet_repository
 
-      commit_attrs = {
-        branch_name: snippet.default_branch,
-        message: 'Update snippet'
-      }
+      attrs = commit_attrs(snippet, UPDATE_COMMIT_MSG)
 
-      snippet.snippet_repository.multi_files_action(current_user, files_to_commit(snippet), commit_attrs)
+      snippet.snippet_repository.multi_files_action(current_user, files_to_commit(snippet), attrs)
     end
 
     # Because we are removing repositories we don't want to remove

changelogs/unreleased/239327-fj-fix-snippet-update-edge-case.yml

+---
+title: Fix edge case when updating snippet with no repo
+merge_request: 42964
+author:
+type: fixed

spec/services/snippets/update_service_spec.rb

@@ -3,7 +3,7 @@
 require 'spec_helper'
 
 RSpec.describe Snippets::UpdateService do
-  describe '#execute' do
+  describe '#execute', :aggregate_failures do
     let_it_be(:user) { create(:user) }
     let_it_be(:admin) { create :user, admin: true }
     let(:visibility_level) { Gitlab::VisibilityLevel::PRIVATE }
@@ -97,40 +97,81 @@ RSpec.describe Snippets::UpdateService do
     end
 
     shared_examples 'creates repository and creates file' do
-      it 'creates repository' do
-        expect(snippet.repository).not_to exist
+      context 'when file_name and content params are used' do
+        it 'creates repository' do
+          expect(snippet.repository).not_to exist
 
-        subject
+          subject
 
-        expect(snippet.repository).to exist
-      end
+          expect(snippet.repository).to exist
+        end
 
-      it 'commits the files to the repository' do
-        subject
+        it 'commits the files to the repository' do
+          subject
 
-        expect(snippet.blobs.count).to eq 1
+          expect(snippet.blobs.count).to eq 1
 
-        blob = snippet.repository.blob_at('master', options[:file_name])
+          blob = snippet.repository.blob_at('master', options[:file_name])
 
-        expect(blob.data).to eq options[:content]
+          expect(blob.data).to eq options[:content]
+        end
+
+        context 'when the repository creation fails' do
+          before do
+            allow(snippet).to receive(:repository_exists?).and_return(false)
+          end
+
+          it 'raise an error' do
+            expect(subject).to be_error
+            expect(subject.payload[:snippet].errors[:repository].to_sentence).to eq 'Error updating the snippet - Repository could not be created'
+          end
+
+          it 'does not try to commit file' do
+            expect(service).not_to receive(:create_commit)
+
+            subject
+          end
+        end
       end
 
-      context 'when the repository creation fails' do
-        before do
-          allow(snippet).to receive(:repository_exists?).and_return(false)
+      context 'when snippet_actions param is used' do
+        let(:file_path) { 'CHANGELOG' }
+        let(:created_file_path) { 'New file'}
+        let(:content) { 'foobar' }
+        let(:snippet_actions) { [{ action: :move, previous_path: snippet.file_name, file_path: file_path }, { action: :create, file_path: created_file_path, content: content }] }
+        let(:base_opts) do
+          {
+            snippet_actions: snippet_actions
+          }
         end
 
-        it 'raise an error' do
-          response = subject
+        it 'performs operation without raising errors' do
+          db_content = snippet.content
 
-          expect(response).to be_error
-          expect(response.payload[:snippet].errors[:repository].to_sentence).to eq 'Error updating the snippet - Repository could not be created'
+          expect(subject).to be_success
+
+          new_blob = snippet.repository.blob_at('master', file_path)
+          created_file = snippet.repository.blob_at('master', created_file_path)
+
+          expect(new_blob.data).to eq db_content
+          expect(created_file.data).to eq content
         end
 
-        it 'does not try to commit file' do
-          expect(service).not_to receive(:create_commit)
+        context 'when the repository is not created' do
+          it 'keeps snippet database data' do
+            old_file_name = snippet.file_name
+            old_file_content = snippet.content
 
-          subject
+            expect_next_instance_of(described_class) do |instance|
+              expect(instance).to receive(:create_repository_for).and_raise(StandardError)
+            end
+
+            snippet = subject.payload[:snippet]
+
+            expect(subject).to be_error
+            expect(snippet.file_name).to eq(old_file_name)
+            expect(snippet.content).to eq(old_file_content)
+          end
         end
       end
     end
@@ -366,10 +407,9 @@ RSpec.describe Snippets::UpdateService do
         let(:snippet_actions) { [{ action: 'invalid_action' }] }
 
         it 'raises a validation error' do
-          response = subject
-          snippet = response.payload[:snippet]
+          snippet = subject.payload[:snippet]
 
-          expect(response).to be_error
+          expect(subject).to be_error
           expect(snippet.errors.full_messages_for(:snippet_actions)).to eq ['Snippet actions have invalid data']
         end
       end
@@ -377,13 +417,12 @@ RSpec.describe Snippets::UpdateService do
       context 'when an error is raised committing the file' do
         it 'keeps any snippet modifications' do
           expect_next_instance_of(described_class) do |instance|
-            expect(instance).to receive(:create_repository_for).and_raise(StandardError)
+            expect(instance).to receive(:create_commit).and_raise(StandardError)
           end
 
-          response = subject
-          snippet = response.payload[:snippet]
+          snippet = subject.payload[:snippet]
 
-          expect(response).to be_error
+          expect(subject).to be_error
           expect(snippet.title).to eq(new_title)
           expect(snippet.file_name).to eq(file_path)
           expect(snippet.content).to eq(content)