Merge branch 'id-refactor-handle-merge-request-errors' into 'master'

Refactor handle_merge_request_errors! method

See merge request gitlab-org/gitlab!36600

ee/lib/ee/api/merge_request_approvals.rb

@@ -8,6 +8,8 @@ module EE
       prepended do
         before { authenticate_non_get! }
 
+        helpers ::API::Helpers::MergeRequestsHelpers
+
         helpers do
           params :ee_approval_params do
             optional :approval_password, type: String, desc: 'Current user\'s password if project is set to require explicit auth on approval'
@@ -17,22 +19,6 @@ module EE
             present merge_request.approval_state, with: ::EE::API::Entities::ApprovalState, current_user: current_user
           end
 
-          def handle_merge_request_errors!(errors)
-            if errors.has_key? :project_access
-              error!(errors[:project_access], 422)
-            elsif errors.has_key? :branch_conflict
-              error!(errors[:branch_conflict], 422)
-            elsif errors.has_key? :validate_fork
-              error!(errors[:validate_fork], 422)
-            elsif errors.has_key? :validate_branches
-              conflict!(errors[:validate_branches])
-            elsif errors.has_key? :base
-              error!(errors[:base], 422)
-            end
-
-            render_api_error!(errors, 400)
-          end
-
           def present_merge_request_approval_state(presenter:, target_branch: nil)
             merge_request = find_merge_request_with_access(params[:merge_request_iid])
 
@@ -71,6 +57,7 @@ module EE
               present_merge_request_approval_state(presenter: ::EE::API::Entities::MergeRequestApprovalState)
             end
 
+            # Deprecated in favor of approval rules API
             desc 'Change approval-related configuration' do
               detail 'This feature was introduced in 10.6'
               success ::EE::API::Entities::ApprovalState
@@ -85,13 +72,13 @@ module EE
 
               merge_request = ::MergeRequests::UpdateService.new(user_project, current_user, approvals_before_merge: params[:approvals_required]).execute(merge_request)
 
-              if merge_request.valid?
+              # Merge request shouldn't be in an invalid state after the changes, but handling errors to be safe
+              handle_merge_request_errors!(merge_request)
+
               present_approval(merge_request)
-              else
-                handle_merge_request_errors! merge_request.errors
-              end
             end
 
+            # Deprecated in favor of approval rules API
             desc 'Update approvers and approver groups' do
               detail 'This feature was introduced in 10.6'
               success ::EE::API::Entities::ApprovalState
@@ -109,11 +96,10 @@ module EE
 
               merge_request = ::MergeRequests::UpdateService.new(user_project, current_user, declared(params, include_parent_namespaces: false).merge(remove_old_approvers: true)).execute(merge_request)
 
-              if merge_request.valid?
+              # Merge request shouldn't be in an invalid state after the changes, but handling errors to be safe
+              handle_merge_request_errors!(merge_request)
+
               present_approval(merge_request)
-              else
-                handle_merge_request_errors! merge_request.errors
-              end
             end
           end
         end

lib/api/helpers.rb

@@ -404,6 +404,10 @@ module API
       render_api_error!(message || '409 Conflict', 409)
     end
 
+    def unprocessable_entity!(message = nil)
+      render_api_error!(message || '422 Unprocessable Entity', :unprocessable_entity)
+    end
+
     def file_too_large!
       render_api_error!('413 Request Entity Too Large', 413)
     end

lib/api/helpers/merge_requests_helpers.rb

@@ -4,6 +4,9 @@ module API
   module Helpers
     module MergeRequestsHelpers
       extend Grape::API::Helpers
+      extend ActiveSupport::Concern
+
+      UNPROCESSABLE_ERROR_KEYS = [:project_access, :branch_conflict, :validate_fork, :base].freeze
 
       params :merge_requests_negatable_params do
         optional :author_id, type: Integer, desc: 'Return merge requests which are authored by the user with the given ID'
@@ -79,6 +82,20 @@ module API
                  default: 'created_by_me',
                  desc: 'Return merge requests for the given scope: `created_by_me`, `assigned_to_me` or `all`'
       end
+
+      def handle_merge_request_errors!(merge_request)
+        return if merge_request.valid?
+
+        errors = merge_request.errors
+
+        UNPROCESSABLE_ERROR_KEYS.each do |error|
+          unprocessable_entity!(errors[error]) if errors.has_key?(error)
+        end
+
+        conflict!(errors[:validate_branches]) if errors.has_key?(:validate_branches)
+
+        render_validation_error!(merge_request)
+      end
     end
   end
 end

lib/api/merge_requests.rb

@@ -153,22 +153,6 @@ module API
       include TimeTrackingEndpoints
 
       helpers do
-        def handle_merge_request_errors!(errors)
-          if errors[:project_access].any?
-            error!(errors[:project_access], 422)
-          elsif errors[:branch_conflict].any?
-            error!(errors[:branch_conflict], 422)
-          elsif errors[:validate_fork].any?
-            error!(errors[:validate_fork], 422)
-          elsif errors[:validate_branches].any?
-            conflict!(errors[:validate_branches])
-          elsif errors[:base].any?
-            error!(errors[:base], 422)
-          end
-
-          render_api_error!(errors, 400)
-        end
-
         params :optional_params do
           optional :description, type: String, desc: 'The description of the merge request'
           optional :assignee_id, type: Integer, desc: 'The ID of a user to assign the merge request'
@@ -226,11 +210,9 @@ module API
 
         merge_request = ::MergeRequests::CreateService.new(user_project, current_user, mr_params).execute
 
-        if merge_request.valid?
+        handle_merge_request_errors!(merge_request)
+
         present merge_request, with: Entities::MergeRequest, current_user: current_user, project: user_project
-        else
-          handle_merge_request_errors! merge_request.errors
-        end
       end
 
       desc 'Delete a merge request'
@@ -420,11 +402,9 @@ module API
 
         merge_request = ::MergeRequests::UpdateService.new(user_project, current_user, mr_params).execute(merge_request)
 
-        if merge_request.valid?
+        handle_merge_request_errors!(merge_request)
+
         present merge_request, with: Entities::MergeRequest, current_user: current_user, project: user_project
-        else
-          handle_merge_request_errors! merge_request.errors
-        end
       end
 
       desc 'Merge a merge request' do

spec/lib/api/helpers/merge_requests_helpers_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe API::Helpers::MergeRequestsHelpers do
+  describe '#handle_merge_request_errors!' do
+    let(:helper) do
+      Class.new do
+        include API::Helpers::MergeRequestsHelpers
+      end.new
+    end
+
+    let(:merge_request) { double }
+
+    context 'when merge request is valid' do
+      it 'returns nil' do
+        allow(merge_request).to receive(:valid?).and_return(true)
+
+        expect(merge_request).not_to receive(:errors)
+
+        helper.handle_merge_request_errors!(merge_request)
+      end
+    end
+
+    context 'when merge request is invalid' do
+      before do
+        allow(merge_request).to receive(:valid?).and_return(false)
+        allow(helper).to receive_messages([
+          :unprocessable_entity!, :conflict!, :render_validation_error!
+        ])
+      end
+
+      API::Helpers::MergeRequestsHelpers::UNPROCESSABLE_ERROR_KEYS.each do |error_key|
+        it "responds to a #{error_key} error with unprocessable_entity" do
+          error = double
+          allow(merge_request).to receive(:errors).and_return({ error_key => error })
+
+          expect(helper).to receive(:unprocessable_entity!).with(error)
+
+          helper.handle_merge_request_errors!(merge_request)
+        end
+      end
+
+      it 'responds to a validate_branches error with conflict' do
+        error = double
+        allow(merge_request).to receive(:errors).and_return({ validate_branches: error })
+
+        expect(helper).to receive(:conflict!).with(error)
+
+        helper.handle_merge_request_errors!(merge_request)
+      end
+
+      it 'responds with bad request' do
+        error = double
+        allow(merge_request).to receive(:errors).and_return({ other_error: error })
+
+        expect(helper).to receive(:render_validation_error!).with(merge_request)
+
+        helper.handle_merge_request_errors!(merge_request)
+      end
+    end
+  end
+end

spec/requests/api/merge_requests_spec.rb

@@ -1551,25 +1551,33 @@ RSpec.describe API::MergeRequests do
       it "returns 422 when source_branch equals target_branch" do
         post api("/projects/#{project.id}/merge_requests", user),
         params: { title: "Test merge_request", source_branch: "master", target_branch: "master", author: user }
+
         expect(response).to have_gitlab_http_status(:unprocessable_entity)
+        expect(json_response['message']).to eq(["You can't use same project/branch for source and target"])
       end
 
       it "returns 400 when source_branch is missing" do
         post api("/projects/#{project.id}/merge_requests", user),
         params: { title: "Test merge_request", target_branch: "master", author: user }
+
         expect(response).to have_gitlab_http_status(:bad_request)
+        expect(json_response['error']).to eq('source_branch is missing')
       end
 
       it "returns 400 when target_branch is missing" do
         post api("/projects/#{project.id}/merge_requests", user),
         params: { title: "Test merge_request", source_branch: "markdown", author: user }
+
         expect(response).to have_gitlab_http_status(:bad_request)
+        expect(json_response['error']).to eq('target_branch is missing')
       end
 
       it "returns 400 when title is missing" do
         post api("/projects/#{project.id}/merge_requests", user),
         params: { target_branch: 'master', source_branch: 'markdown' }
+
         expect(response).to have_gitlab_http_status(:bad_request)
+        expect(json_response['error']).to eq('title is missing')
       end
 
       context 'with existing MR' do
@@ -1594,7 +1602,9 @@ RSpec.describe API::MergeRequests do
                    author: user
                  }
           end.to change { MergeRequest.count }.by(0)
+
           expect(response).to have_gitlab_http_status(:conflict)
+          expect(json_response['message']).to eq(["Another open merge request already exists for this source branch: !5"])
         end
       end