Merge branch 'al-214695-improve-response-in-snippets-api' into 'master'

Improve responses in the snippet create/update API endpoints

See merge request gitlab-org/gitlab!32282

changelogs/unreleased/al-214695-improve-response-in-snippets-api.yml

+---
+title: Improve responses in the snippet create/update API endpoints
+merge_request: 32282
+author:
+type: fixed

lib/api/project_snippets.rb

@@ -70,12 +70,12 @@ module API
         service_response = ::Snippets::CreateService.new(user_project, current_user, snippet_params).execute
         snippet = service_response.payload[:snippet]
 
-        render_spam_error! if snippet.spam?
-
-        if snippet.persisted?
+        if service_response.success?
           present snippet, with: Entities::ProjectSnippet
         else
-          render_validation_error!(snippet)
+          render_spam_error! if snippet.spam?
+
+          render_api_error!({ error: service_response.message }, service_response.http_status)
         end
       end
 
@@ -106,12 +106,12 @@ module API
         service_response = ::Snippets::UpdateService.new(user_project, current_user, snippet_params).execute(snippet)
         snippet = service_response.payload[:snippet]
 
-        render_spam_error! if snippet.spam?
-
-        if snippet.valid?
+        if service_response.success?
           present snippet, with: Entities::ProjectSnippet
         else
-          render_validation_error!(snippet)
+          render_spam_error! if snippet.spam?
+
+          render_api_error!({ error: service_response.message }, service_response.http_status)
         end
       end
       # rubocop: enable CodeReuse/ActiveRecord

lib/api/snippets.rb

@@ -81,12 +81,12 @@ module API
         service_response = ::Snippets::CreateService.new(nil, current_user, attrs).execute
         snippet = service_response.payload[:snippet]
 
-        render_spam_error! if snippet.spam?
-
-        if snippet.persisted?
+        if service_response.success?
           present snippet, with: Entities::PersonalSnippet
         else
-          render_validation_error!(snippet)
+          render_spam_error! if snippet.spam?
+
+          render_api_error!({ error: service_response.message }, service_response.http_status)
         end
       end
 
@@ -115,12 +115,12 @@ module API
         service_response = ::Snippets::UpdateService.new(nil, current_user, attrs).execute(snippet)
         snippet = service_response.payload[:snippet]
 
-        render_spam_error! if snippet.spam?
-
-        if snippet.persisted?
+        if service_response.success?
           present snippet, with: Entities::PersonalSnippet
         else
-          render_validation_error!(snippet)
+          render_spam_error! if snippet.spam?
+
+          render_api_error!({ error: service_response.message }, service_response.http_status)
         end
       end
 

spec/requests/api/project_snippets_spec.rb

@@ -224,6 +224,20 @@ describe API::ProjectSnippets do
       expect(response).to have_gitlab_http_status(:bad_request)
     end
 
+    context 'when save fails because the repository could not be created' do
+      before do
+        allow_next_instance_of(Snippets::CreateService) do |instance|
+          allow(instance).to receive(:create_repository).and_raise(Snippets::CreateService::CreateRepositoryError)
+        end
+      end
+
+      it 'returns 400' do
+        post api("/projects/#{project.id}/snippets", admin), params: params
+
+        expect(response).to have_gitlab_http_status(:bad_request)
+      end
+    end
+
     context 'when the snippet is spam' do
       def create_snippet(project, snippet_params = {})
         project.add_developer(user)

spec/requests/api/snippets_spec.rb

@@ -267,6 +267,28 @@ describe API::Snippets do
       expect(response).to have_gitlab_http_status(:bad_request)
     end
 
+    it 'returns 400 for validation errors' do
+      params[:title] = ''
+
+      post api("/snippets/", user), params: params
+
+      expect(response).to have_gitlab_http_status(:bad_request)
+    end
+
+    context 'when save fails because the repository could not be created' do
+      before do
+        allow_next_instance_of(Snippets::CreateService) do |instance|
+          allow(instance).to receive(:create_repository).and_raise(Snippets::CreateService::CreateRepositoryError)
+        end
+      end
+
+      it 'returns 400' do
+        post api("/snippets/", user), params: params
+
+        expect(response).to have_gitlab_http_status(:bad_request)
+      end
+    end
+
     context 'when the snippet is spam' do
       def create_snippet(snippet_params = {})
         post api('/snippets', user), params: params.merge(snippet_params)
@@ -356,6 +378,12 @@ describe API::Snippets do
       expect(response).to have_gitlab_http_status(:bad_request)
     end
 
+    it 'returns 400 for validation errors' do
+      update_snippet(params: { title: '' })
+
+      expect(response).to have_gitlab_http_status(:bad_request)
+    end
+
     it_behaves_like 'update with repository actions' do
       let(:snippet_without_repo) { create(:personal_snippet, author: user, visibility_level: visibility_level) }
     end

spec/support/shared_examples/requests/snippet_shared_examples.rb

@@ -48,6 +48,28 @@ RSpec.shared_examples 'update with repository actions' do
         expect(blob).not_to be_nil
         expect(blob.data).to eq content
       end
+
+      context 'when save fails due to a repository creation error' do
+        let(:content) { 'File content' }
+        let(:file_name) { 'test.md' }
+
+        before do
+          allow_next_instance_of(Snippets::UpdateService) do |instance|
+            allow(instance).to receive(:create_repository_for).with(snippet).and_raise(Snippets::UpdateService::CreateRepositoryError)
+          end
+
+          update_snippet(snippet_id: snippet.id, params: { content: content, file_name: file_name })
+        end
+
+        it 'returns 400' do
+          expect(response).to have_gitlab_http_status(:bad_request)
+        end
+
+        it 'does not save the changes to the snippet object' do
+          expect(snippet.content).not_to eq(content)
+          expect(snippet.file_name).not_to eq(file_name)
+        end
+      end
     end
   end
 end