Merge branch 'fj-fix-single-param-update' into 'master'

Fix bug in snippets updating only file_name or content

See merge request gitlab-org/gitlab!33375

app/models/snippet_repository.rb

@@ -53,10 +53,21 @@ class SnippetRepository < ApplicationRecord
   def transform_file_entries(files)
     next_index = get_last_empty_file_index + 1
 
-    files.each do |file_entry|
+    files.map do |file_entry|
       file_entry[:file_path] = file_path_for(file_entry, next_index) { next_index += 1 }
       file_entry[:action] = infer_action(file_entry) unless file_entry[:action]
-    end
+      file_entry[:action] = file_entry[:action].to_sym
+
+      if only_rename_action?(file_entry)
+        file_entry[:infer_content] = true
+      elsif empty_update_action?(file_entry)
+        # There is no need to perform a repository operation
+        # When the update action has no content
+        file_entry = nil
+      end
+
+      file_entry
+    end.compact
   end
 
   def file_path_for(file_entry, next_index)
@@ -111,4 +122,12 @@ class SnippetRepository < ApplicationRecord
     err.is_a?(ArgumentError) &&
       err.message.downcase.match?(/failed to parse signature/)
   end
+
+  def only_rename_action?(action)
+    action[:action] == :move && action[:content].nil?
+  end
+
+  def empty_update_action?(action)
+    action[:action] == :update && action[:content].nil?
+  end
 end

app/services/snippets/update_service.rb

@@ -112,8 +112,10 @@ module Snippets
     end
 
     def build_actions_from_params(snippet)
-      [{ previous_path: snippet.file_name_on_repo,
-         file_path: params[:file_name],
+      file_name_on_repo = snippet.file_name_on_repo
+
+      [{ previous_path: file_name_on_repo,
+         file_path: params[:file_name] || file_name_on_repo,
          content: params[:content] }]
     end
   end

changelogs/unreleased/fj-fix-single-param-update.yml

+---
+title: Fix bug in snippets updating only file_name or content
+merge_request: 33375
+author:
+type: fixed

spec/models/snippet_repository_spec.rb

@@ -116,20 +116,84 @@ describe SnippetRepository do
       end
 
       context 'when commit actions are present' do
-        let(:file_action) { { file_path: 'foo.txt', content: 'foo', action: :foobar } }
-        let(:data) { [file_action] }
+        shared_examples 'uses the expected action' do |action, expected_action|
+          let(:file_action) { { file_path: 'foo.txt', content: 'foo', action: action } }
+          let(:data) { [file_action] }
+
+          specify do
+            expect(repo).to(
+              receive(:multi_action).with(
+                user,
+                hash_including(actions: array_including(hash_including(action: expected_action)))))
+
+            snippet_repository.multi_files_action(user, data, commit_opts)
+          end
+        end
 
-        it 'does not change commit action' do
-          expect(repo).to(
-            receive(:multi_action).with(
-              user,
-              hash_including(actions: array_including(hash_including(action: :foobar)))))
+        it_behaves_like 'uses the expected action', :foobar, :foobar
 
-          snippet_repository.multi_files_action(user, data, commit_opts)
+        context 'when action is a string' do
+          it_behaves_like 'uses the expected action', 'foobar', :foobar
         end
       end
     end
 
+    context 'when move action does not include content' do
+      let(:previous_path) { 'CHANGELOG' }
+      let(:new_path) { 'CHANGELOG_new' }
+      let(:move_action) { { previous_path: previous_path, file_path: new_path, action: action } }
+
+      shared_examples 'renames file and does not update content' do
+        specify do
+          existing_content = blob_at(snippet, previous_path).data
+
+          snippet_repository.multi_files_action(user, [move_action], commit_opts)
+
+          blob = blob_at(snippet, new_path)
+          expect(blob).not_to be_nil
+          expect(blob.data).to eq existing_content
+        end
+      end
+
+      context 'when action is not set' do
+        let(:action) { nil }
+
+        it_behaves_like 'renames file and does not update content'
+      end
+
+      context 'when action is set' do
+        let(:action) { :move }
+
+        it_behaves_like 'renames file and does not update content'
+      end
+    end
+
+    context 'when update action does not include content' do
+      let(:update_action) { { previous_path: 'CHANGELOG', file_path: 'CHANGELOG', action: action } }
+
+      shared_examples 'does not commit anything' do
+        specify do
+          last_commit_id = snippet.repository.head_commit.id
+
+          snippet_repository.multi_files_action(user, [update_action], commit_opts)
+
+          expect(snippet.repository.head_commit.id).to eq last_commit_id
+        end
+      end
+
+      context 'when action is not set' do
+        let(:action) { nil }
+
+        it_behaves_like 'does not commit anything'
+      end
+
+      context 'when action is set' do
+        let(:action) { :update }
+
+        it_behaves_like 'does not commit anything'
+      end
+    end
+
     shared_examples 'snippet repository with file names' do |*filenames|
       it 'sets a name for unnamed files' do
         ls_files = snippet.repository.ls_files(nil)

spec/services/snippets/update_service_spec.rb

@@ -378,6 +378,60 @@ describe Snippets::UpdateService do
       end
     end
 
+    shared_examples 'only file_name is present' do
+      let(:base_opts) do
+        {
+          file_name: file_name
+        }
+      end
+
+      shared_examples 'content is not updated' do
+        specify do
+          existing_content = snippet.blobs.first.data
+          response = subject
+          snippet = response.payload[:snippet]
+
+          blob = snippet.repository.blob_at('master', file_name)
+
+          expect(blob).not_to be_nil
+          expect(response).to be_success
+          expect(blob.data).to eq existing_content
+        end
+      end
+
+      context 'when renaming the file_name' do
+        let(:file_name) { 'new_file_name' }
+
+        it_behaves_like 'content is not updated'
+      end
+
+      context 'when file_name does not change' do
+        let(:file_name) { snippet.blobs.first.path }
+
+        it_behaves_like 'content is not updated'
+      end
+    end
+
+    shared_examples 'only content is present' do
+      let(:content) { 'new_content' }
+      let(:base_opts) do
+        {
+          content: content
+        }
+      end
+
+      it 'updates the content' do
+        response = subject
+        snippet = response.payload[:snippet]
+
+        blob = snippet.repository.blob_at('master', snippet.blobs.first.path)
+
+        expect(blob).not_to be_nil
+        expect(response).to be_success
+        expect(blob.data).to eq content
+      end
+    end
+
     context 'when Project Snippet' do
       let_it_be(:project) { create(:project) }
       let!(:snippet) { create(:project_snippet, :repository, author: user, project: project) }
@@ -393,6 +447,8 @@ describe Snippets::UpdateService do
       it_behaves_like 'commit operation fails'
       it_behaves_like 'committable attributes'
       it_behaves_like 'when snippet_files param is present'
+      it_behaves_like 'only file_name is present'
+      it_behaves_like 'only content is present'
       it_behaves_like 'snippets spam check is performed' do
         before do
           subject
@@ -418,6 +474,8 @@ describe Snippets::UpdateService do
       it_behaves_like 'commit operation fails'
       it_behaves_like 'committable attributes'
       it_behaves_like 'when snippet_files param is present'
+      it_behaves_like 'only file_name is present'
+      it_behaves_like 'only content is present'
       it_behaves_like 'snippets spam check is performed' do
         before do
           subject