Merge branch 'fj-214692-avoid-commit-when-content-file-name-not-updated' into 'master'

Avoid commit when snippet file_name and content are not present

Closes #214683 and #214692

See merge request gitlab-org/gitlab!29761

app/services/snippets/update_service.rb

@@ -4,6 +4,8 @@ module Snippets
   class UpdateService < Snippets::BaseService
     include SpamCheckMethods
 
+    COMMITTABLE_ATTRIBUTES = %w(file_name content).freeze
+
     UpdateError = Class.new(StandardError)
     CreateRepositoryError = Class.new(StandardError)
 
@@ -37,6 +39,10 @@ module Snippets
     def save_and_commit(snippet)
       return false unless snippet.save
 
+      # If the updated attributes does not need to update
+      # the repository we can just return
+      return true unless committable_attributes?
+
       # In order to avoid non migrated snippets scenarios,
       # if the snippet does not have a repository we created it
       # We don't need to check if the repository exists
@@ -104,5 +110,9 @@ module Snippets
     def repository_empty?(snippet)
       snippet.repository._uncached_exists? && !snippet.repository._uncached_has_visible_content?
     end
+
+    def committable_attributes?
+      (params.stringify_keys.keys & COMMITTABLE_ATTRIBUTES).present?
+    end
   end
 end

changelogs/unreleased/fj-214692-avoid-commit-when-content-file-name-not-updated.yml

+---
+title: Avoid commit when snippet file_name and content are not present
+merge_request: 29761
+author:
+type: changed

spec/features/projects/snippets/user_updates_snippet_spec.rb

@@ -61,6 +61,7 @@ describe 'Projects > Snippets > User updates a snippet', :js do
       end
 
       fill_in('project_snippet_title', with: 'Snippet new title')
+      fill_in('project_snippet_file_name', with: 'new_file_name')
 
       click_button('Save')
     end

spec/features/snippets/user_edits_snippet_spec.rb

@@ -92,6 +92,7 @@ describe 'User edits snippet', :js do
       end
 
       fill_in 'personal_snippet_title', with: 'New Snippet Title'
+      fill_in 'personal_snippet_file_name', with: 'new_file_name'
 
       click_button('Save changes')
     end

spec/requests/api/snippets_spec.rb

@@ -424,6 +424,32 @@ describe API::Snippets do
       end
     end
 
+    context "when admin" do
+      let(:admin) { create(:admin) }
+      let(:token) { create(:personal_access_token, user: admin, scopes: [:sudo]) }
+
+      subject do
+        put api("/snippets/#{snippet.id}", admin, personal_access_token: token), params: { visibility: 'private', sudo: user.id }
+      end
+
+      context 'when sudo is defined' do
+        it 'returns 200 and updates snippet visibility' do
+          expect(snippet.visibility).not_to eq('private')
+
+          subject
+
+          expect(response).to have_gitlab_http_status(:success)
+          expect(json_response["visibility"]).to eq 'private'
+        end
+
+        it 'does not commit data' do
+          expect_any_instance_of(SnippetRepository).not_to receive(:multi_files_action)
+
+          subject
+        end
+      end
+    end
+
     def update_snippet(snippet_id: snippet.id, params: {}, requester: user)
       put api("/snippets/#{snippet_id}", requester), params: params
     end

spec/services/snippets/update_service_spec.rb

@@ -270,6 +270,35 @@ describe Snippets::UpdateService do
       end
     end
 
+    shared_examples 'committable attributes' do
+      context 'when file_name is updated' do
+        let(:options) { { file_name: 'snippet.rb' } }
+
+        it 'commits to repository' do
+          expect(service).to receive(:create_commit)
+          expect(subject).to be_success
+        end
+      end
+
+      context 'when content is updated' do
+        let(:options) { { content: 'puts "hello world"' } }
+
+        it 'commits to repository' do
+          expect(service).to receive(:create_commit)
+          expect(subject).to be_success
+        end
+      end
+
+      context 'when content or file_name is not updated' do
+        let(:options) { { title: 'Test snippet' } }
+
+        it 'does not perform any commit' do
+          expect(service).not_to receive(:create_commit)
+          expect(subject).to be_success
+        end
+      end
+    end
+
     context 'when Project Snippet' do
       let_it_be(:project) { create(:project) }
       let!(:snippet) { create(:project_snippet, :repository, author: user, project: project) }
@@ -283,6 +312,7 @@ describe Snippets::UpdateService do
       it_behaves_like 'snippet update data is tracked'
       it_behaves_like 'updates repository content'
       it_behaves_like 'commit operation fails'
+      it_behaves_like 'committable attributes'
 
       context 'when snippet does not have a repository' do
         let!(:snippet) { create(:project_snippet, author: user, project: project) }
@@ -301,6 +331,7 @@ describe Snippets::UpdateService do
       it_behaves_like 'snippet update data is tracked'
       it_behaves_like 'updates repository content'
       it_behaves_like 'commit operation fails'
+      it_behaves_like 'committable attributes'
 
       context 'when snippet does not have a repository' do
         let!(:snippet) { create(:personal_snippet, author: user, project: project) }

spec/support/shared_examples/requests/snippet_shared_examples.rb

@@ -23,21 +23,31 @@ RSpec.shared_examples 'update with repository actions' do
   context 'when the repository does not exist' do
     let(:snippet) { snippet_without_repo }
 
-    it 'creates the repository' do
-      update_snippet(snippet_id: snippet.id, params: { title: 'foo' })
+    context 'when update attributes does not include file_name or content' do
+      it 'does not create the repository' do
+        update_snippet(snippet_id: snippet.id, params: { title: 'foo' })
 
-      expect(snippet.repository).to exist
+        expect(snippet.repository).not_to exist
+      end
     end
 
-    it 'commits the file to the repository' do
-      content = 'New Content'
-      file_name = 'file_name.rb'
+    context 'when update attributes include file_name or content' do
+      it 'creates the repository' do
+        update_snippet(snippet_id: snippet.id, params: { title: 'foo', file_name: 'foo' })
+
+        expect(snippet.repository).to exist
+      end
+
+      it 'commits the file to the repository' do
+        content = 'New Content'
+        file_name = 'file_name.rb'
 
-      update_snippet(snippet_id: snippet.id, params: { content: content, file_name: file_name })
+        update_snippet(snippet_id: snippet.id, params: { content: content, file_name: file_name })
 
-      blob = snippet.repository.blob_at('master', file_name)
-      expect(blob).not_to be_nil
-      expect(blob.data).to eq content
+        blob = snippet.repository.blob_at('master', file_name)
+        expect(blob).not_to be_nil
+        expect(blob.data).to eq content
+      end
     end
   end
 end