Merge branch 'vij-snippet-rest-update' into 'master'

PersonalSnippet update with files support

See merge request gitlab-org/gitlab!40084

app/models/snippet.rb

@@ -345,6 +345,10 @@ class Snippet < ApplicationRecord
     repository.ls_files(ref)
   end
 
+  def multiple_files?
+    list_files(repository.root_ref).size > 1
+  end
+
   class << self
     # Searches for snippets with a matching title, description or file name.
     #

lib/api/helpers.rb

@@ -537,6 +537,10 @@ module API
       )
     end
 
+    def with_api_params(&block)
+      yield({ api: true, request: request })
+    end
+
     protected
 
     def project_finder_params_visibility_ce

lib/api/helpers/snippets_helpers.rb

@@ -27,6 +27,20 @@ module API
         exactly_one_of :files, :content
       end
 
+      params :update_file_params do |options|
+        optional :files, type: Array, desc: 'An array of files to update' do
+          requires :action, type: String,
+            values: SnippetInputAction::ACTIONS.map(&:to_s),
+            desc: "The type of action to perform on the file, must be one of: #{SnippetInputAction::ACTIONS.join(", ")}"
+          optional :content, type: String, desc: 'The content of a snippet'
+          optional :file_path, file_path: true, type: String, desc: 'The file path of a snippet file'
+          optional :previous_path, file_path: true, type: String, desc: 'The previous path of a snippet file'
+        end
+
+        mutually_exclusive :files, :content
+        mutually_exclusive :files, :file_name
+      end
+
       def content_for(snippet)
         if snippet.empty_repo?
           env['api.format'] = :txt
@@ -53,10 +67,30 @@ module API
       end
     end
 
-    def process_file_args(args)
-      args[:snippet_actions] = args.delete(:files)&.map do |file|
-        file[:action] = :create
-        file.symbolize_keys
+    def process_create_params(args)
+      with_api_params do |api_params|
+        args[:snippet_actions] = args.delete(:files)&.map do |file|
+          file[:action] = :create
+          file.symbolize_keys
+        end
+
+        args.merge(api_params)
+      end
+    end
+
+    def process_update_params(args)
+      with_api_params do |api_params|
+        args[:snippet_actions] = args.delete(:files)&.map(&:symbolize_keys)
+
+        args.merge(api_params)
+      end
+    end
+
+    def validate_params_for_multiple_files(snippet)
+      return unless params[:content] || params[:file_name]
+
+      if Feature.enabled?(:snippet_multiple_files, current_user) && snippet.multiple_files?
+        render_api_error!({ error: _('To update Snippets with multiple files, you must use the `files` parameter') }, 400)
       end
     end
   end

lib/api/project_snippets.rb

@@ -64,12 +64,8 @@ module API
       end
       post ":id/snippets" do
         authorize! :create_snippet, user_project
-        snippet_params = declared_params(include_missing: false).tap do |create_args|
-          create_args[:request] = request
-          create_args[:api] = true
 
-          process_file_args(create_args)
-        end
+        snippet_params = process_create_params(declared_params(include_missing: false))
 
         service_response = ::Snippets::CreateService.new(user_project, current_user, snippet_params).execute
         snippet = service_response.payload[:snippet]

lib/api/snippets.rb

@@ -76,12 +76,7 @@ module API
       post do
         authorize! :create_snippet
 
-        attrs = declared_params(include_missing: false).tap do |create_args|
-          create_args[:request] = request
-          create_args[:api] = true
-
-          process_file_args(create_args)
-        end
+        attrs = process_create_params(declared_params(include_missing: false))
 
         service_response = ::Snippets::CreateService.new(nil, current_user, attrs).execute
         snippet = service_response.payload[:snippet]
@@ -99,16 +94,20 @@ module API
         detail 'This feature was introduced in GitLab 8.15.'
         success Entities::PersonalSnippet
       end
+
       params do
         requires :id, type: Integer, desc: 'The ID of a snippet'
-        optional :title, type: String, allow_blank: false, desc: 'The title of a snippet'
-        optional :file_name, type: String, desc: 'The name of a snippet file'
         optional :content, type: String, allow_blank: false, desc: 'The content of a snippet'
         optional :description, type: String, desc: 'The description of a snippet'
+        optional :file_name, type: String, desc: 'The name of a snippet file'
+        optional :title, type: String, allow_blank: false, desc: 'The title of a snippet'
         optional :visibility, type: String,
                               values: Gitlab::VisibilityLevel.string_values,
                               desc: 'The visibility of the snippet'
-        at_least_one_of :title, :file_name, :content, :visibility
+
+        use :update_file_params
+
+        at_least_one_of :title, :file_name, :content, :files, :visibility
       end
       put ':id' do
         snippet = snippets_for_current_user.find_by_id(params.delete(:id))
@@ -116,8 +115,12 @@ module API
 
         authorize! :update_snippet, snippet
 
-        attrs = declared_params(include_missing: false).merge(request: request, api: true)
+        validate_params_for_multiple_files(snippet)
+
+        attrs = process_update_params(declared_params(include_missing: false))
+
         service_response = ::Snippets::UpdateService.new(nil, current_user, attrs).execute(snippet)
+
         snippet = service_response.payload[:snippet]
 
         if service_response.success?

locale/gitlab.pot

@@ -25971,6 +25971,9 @@ msgstr ""
 msgid "To unsubscribe from this issue, please paste the following link into your browser:"
 msgstr ""
 
+msgid "To update Snippets with multiple files, you must use the `files` parameter"
+msgstr ""
+
 msgid "To view all %{scannedResourcesCount} scanned URLs, please download the CSV file"
 msgstr ""
 

spec/models/snippet_spec.rb

@@ -787,4 +787,26 @@ RSpec.describe Snippet do
       end
     end
   end
+
+  describe '#multiple_files?' do
+    subject { snippet.multiple_files? }
+
+    context 'when snippet has multiple files' do
+      let(:snippet) { create(:snippet, :repository) }
+
+      it { is_expected.to be_truthy }
+    end
+
+    context 'when snippet does not have multiple files' do
+      let(:snippet) { create(:snippet, :empty_repo) }
+
+      it { is_expected.to be_falsey }
+    end
+
+    context 'when the snippet does not have a repository' do
+      let(:snippet) { build(:snippet) }
+
+      it { is_expected.to be_falsey }
+    end
+  end
 end

spec/requests/api/snippets_spec.rb

@@ -391,21 +391,98 @@ RSpec.describe API::Snippets do
       create(:personal_snippet, :repository, author: user, visibility_level: visibility_level)
     end
 
-    shared_examples 'snippet updates' do
-      it 'updates a snippet' do
-        new_content = 'New content'
+    let(:create_action)     { { action: 'create', file_path: 'foo.txt', content: 'bar' } }
+    let(:update_action)     { { action: 'update', file_path: 'CHANGELOG', content: 'bar' } }
+    let(:move_action)       { { action: 'move',   file_path: '.old-gitattributes', previous_path: '.gitattributes' } }
+    let(:delete_action)     { { action: 'delete', file_path: 'CONTRIBUTING.md' } }
+    let(:bad_file_path)     { { action: 'create', file_path: '../../etc/passwd', content: 'bar' } }
+    let(:bad_previous_path) { { action: 'create', previous_path: '../../etc/passwd', file_path: 'CHANGELOG', content: 'bar' } }
+    let(:invalid_move)      { { action: 'move',   file_path: 'missing_previous_path.txt' } }
+
+    context 'with snippet file changes' do
+      using RSpec::Parameterized::TableSyntax
+
+      where(:is_multi_file, :file_name, :content, :files, :status) do
+        true  | nil       | nil   | [create_action]                | :success
+        true  | nil       | nil   | [update_action]                | :success
+        true  | nil       | nil   | [move_action]                  | :success
+        true  | nil       | nil   | [delete_action]                | :success
+        true  | nil       | nil   | [create_action, update_action] | :success
+        true  | 'foo.txt' | 'bar' | [create_action]                | :bad_request
+        true  | 'foo.txt' | 'bar' | nil                            | :bad_request
+        true  | nil       | nil   | nil                            | :bad_request
+        true  | 'foo.txt' | nil   | [create_action]                | :bad_request
+        true  | nil       | 'bar' | [create_action]                | :bad_request
+        true  | ''        | nil   | [create_action]                | :bad_request
+        true  | nil       | ''    | [create_action]                | :bad_request
+        true  | nil       | nil   | [bad_file_path]                | :bad_request
+        true  | nil       | nil   | [bad_previous_path]            | :bad_request
+        true  | nil       | nil   | [invalid_move]                 | :forbidden
+
+        false | 'foo.txt' | 'bar' | nil                            | :success
+        false | 'foo.txt' | nil   | nil                            | :success
+        false | nil       | 'bar' | nil                            | :success
+        false | 'foo.txt' | 'bar' | [create_action]                | :bad_request
+        false | nil       | nil   | nil                            | :bad_request
+        false | nil       | ''    | nil                            | :bad_request
+        false | nil       | nil   | [bad_file_path]                | :bad_request
+        false | nil       | nil   | [bad_previous_path]            | :bad_request
+      end
+
+      with_them do
+        before do
+          allow_any_instance_of(Snippet).to receive(:multiple_files?).and_return(is_multi_file)
+        end
+
+        it 'has the correct response' do
+          update_params = {}.tap do |params|
+            params[:files]     = files     if files
+            params[:file_name] = file_name if file_name
+            params[:content]   = content   if content
+          end
+
+          update_snippet(params: update_params)
+
+          expect(response).to have_gitlab_http_status(status)
+        end
+      end
+
+      context 'when save fails due to a repository commit error' do
+        before do
+          allow_next_instance_of(Repository) do |instance|
+            allow(instance).to receive(:multi_action).and_raise(Gitlab::Git::CommitError)
+          end
+
+          update_snippet(params: { files: [create_action] })
+        end
+
+        it 'returns a bad request response' do
+          expect(response).to have_gitlab_http_status(:bad_request)
+        end
+      end
+    end
+
+    shared_examples 'snippet non-file updates' do
+      it 'updates a snippet non-file attributes' do
         new_description = 'New description'
+        new_title       = 'New title'
+        new_visibility  = 'internal'
 
-        update_snippet(params: { content: new_content, description: new_description, visibility: 'internal' })
+        update_snippet(params: { title: new_title, description: new_description, visibility: new_visibility })
 
-        expect(response).to have_gitlab_http_status(:ok)
         snippet.reload
-        expect(snippet.content).to eq(new_content)
-        expect(snippet.description).to eq(new_description)
-        expect(snippet.visibility).to eq('internal')
+
+        aggregate_failures do
+          expect(response).to have_gitlab_http_status(:ok)
+          expect(snippet.description).to eq(new_description)
+          expect(snippet.visibility).to eq(new_visibility)
+          expect(snippet.title).to eq(new_title)
+        end
       end
     end
 
+    it_behaves_like 'snippet non-file updates'
+
     context 'with restricted visibility settings' do
       before do
         stub_application_setting(restricted_visibility_levels:
@@ -413,11 +490,9 @@ RSpec.describe API::Snippets do
                                     Gitlab::VisibilityLevel::PRIVATE])
       end
 
-      it_behaves_like 'snippet updates'
+      it_behaves_like 'snippet non-file updates'
     end
 
-    it_behaves_like 'snippet updates'
-
     it 'returns 404 for invalid snippet id' do
       update_snippet(snippet_id: non_existing_record_id, params: { title: 'Foo' })
 
@@ -438,13 +513,6 @@ RSpec.describe API::Snippets do
       expect(response).to have_gitlab_http_status(:bad_request)
     end
 
-    it 'returns 400 if content is blank' do
-      update_snippet(params: { content: '' })
-
-      expect(response).to have_gitlab_http_status(:bad_request)
-      expect(json_response['error']).to eq 'content is empty'
-    end
-
     it 'returns 400 if title is blank' do
       update_snippet(params: { title: '' })
 

spec/support/shared_examples/requests/snippet_shared_examples.rb

@@ -2,6 +2,10 @@
 
 RSpec.shared_examples 'update with repository actions' do
   context 'when the repository exists' do
+    before do
+      allow_any_instance_of(Snippet).to receive(:multiple_files?).and_return(false)
+    end
+
     it 'commits the changes to the repository' do
       existing_blob = snippet.blobs.first
       new_file_name = existing_blob.path + '_new'