Add payload to the service response

This introduces payload to the ServiceResponse with
the merge ref HEAD commit data

6 files changed, 152 insertions, 16 deletions

diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 38b9b886e5f..cf63a7e79bd 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -1080,10 +1080,6 @@ class MergeRequest < ApplicationRecord
 
   # Returns the current merge-ref HEAD commit.
   #
-  # Consider calling mergeability_check method _before_ this if you need
-  # the latest possible version of it (it's already automatically updated
-  # along the merge_status).
-  #
   def merge_ref_head
     project.repository.commit(merge_ref_path)
   end
diff --git a/app/services/merge_requests/mergeability_check_service.rb b/app/services/merge_requests/mergeability_check_service.rb
index d277d38127c..ef833774e65 100644
--- a/app/services/merge_requests/mergeability_check_service.rb
+++ b/app/services/merge_requests/mergeability_check_service.rb
@@ -2,6 +2,8 @@
 
 module MergeRequests
   class MergeabilityCheckService < ::BaseService
+    include Gitlab::Utils::StrongMemoize
+
     delegate :project, to: :@merge_request
     delegate :repository, to: :project
 
@@ -16,8 +18,8 @@ module MergeRequests
     # and the merge-ref is synced. Success in case of being/becoming mergeable,
     # error otherwise.
     def execute
-      return ServiceResponse.error('Invalid argument') unless merge_request
-      return ServiceResponse.error('Unsupported operation') if Gitlab::Database.read_only?
+      return ServiceResponse.error(message: 'Invalid argument') unless merge_request
+      return ServiceResponse.error(message: 'Unsupported operation') if Gitlab::Database.read_only?
 
       update_merge_status
 
@@ -25,13 +27,39 @@ module MergeRequests
         return ServiceResponse.error(message: 'Merge request is not mergeable')
       end
 
-      ServiceResponse.success
+      unless payload.fetch(:merge_ref_head)
+        return ServiceResponse.error(message: 'Merge ref was not found')
+      end
+
+      ServiceResponse.success(payload: payload)
     end
 
     private
 
     attr_reader :merge_request
 
+    def payload
+      strong_memoize(:payload) do
+        {
+          merge_ref_head: merge_ref_head_payload
+        }
+      end
+    end
+
+    def merge_ref_head_payload
+      commit = merge_request.merge_ref_head
+
+      return unless commit
+
+      target_id, source_id = commit.parent_ids
+
+      {
+        commit_id: commit.id,
+        source_id: source_id,
+        target_id: target_id
+      }
+    end
+
     def update_merge_status
       return unless merge_request.recheck_merge_status?
 
diff --git a/app/services/service_response.rb b/app/services/service_response.rb
index 1de30e68d87..f3437ba16de 100644
--- a/app/services/service_response.rb
+++ b/app/services/service_response.rb
@@ -1,19 +1,20 @@
 # frozen_string_literal: true
 
 class ServiceResponse
-  def self.success(message: nil)
-    new(status: :success, message: message)
+  def self.success(message: nil, payload: {})
+    new(status: :success, message: message, payload: payload)
   end
 
-  def self.error(message:, http_status: nil)
-    new(status: :error, message: message, http_status: http_status)
+  def self.error(message:, payload: {}, http_status: nil)
+    new(status: :error, message: message, payload: payload, http_status: http_status)
   end
 
-  attr_reader :status, :message, :http_status
+  attr_reader :status, :message, :http_status, :payload
 
-  def initialize(status:, message: nil, http_status: nil)
+  def initialize(status:, message: nil, payload: {}, http_status: nil)
     self.status = status
     self.message = message
+    self.payload = payload
     self.http_status = http_status
   end
 
@@ -27,5 +28,5 @@ class ServiceResponse
 
   private
 
-  attr_writer :status, :message, :http_status
+  attr_writer :status, :message, :http_status, :payload
 end
diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb
index a1365d5c02b..0a9fe450500 100644
--- a/lib/api/merge_requests.rb
+++ b/lib/api/merge_requests.rb
@@ -404,8 +404,8 @@ module API
 
         result = ::MergeRequests::MergeabilityCheckService.new(merge_request).execute
 
-        if result.success? && commit = merge_request.merge_ref_head
-          present :commit_id, commit.sha
+        if result.success?
+          present :commit_id, result.payload.dig(:merge_ref_head, :commit_id)
         else
           render_api_error!(result.message, 400)
         end
diff --git a/spec/services/merge_requests/mergeability_check_service_spec.rb b/spec/services/merge_requests/mergeability_check_service_spec.rb
index 6ad079d5f28..aa0485467ed 100644
--- a/spec/services/merge_requests/mergeability_check_service_spec.rb
+++ b/spec/services/merge_requests/mergeability_check_service_spec.rb
@@ -39,6 +39,14 @@ describe MergeRequests::MergeabilityCheckService do
       expect(result).to be_a(ServiceResponse)
       expect(result).to be_success
     end
+
+    it 'ServiceResponse has merge_ref_head payload' do
+      result = subject
+
+      expect(result.payload.keys).to contain_exactly(:merge_ref_head)
+      expect(result.payload[:merge_ref_head].keys)
+        .to contain_exactly(:commit_id, :target_id, :source_id)
+    end
   end
 
   describe '#execute' do
@@ -54,6 +62,21 @@ describe MergeRequests::MergeabilityCheckService do
 
     it_behaves_like 'mergeable merge request'
 
+    context 'when multiple calls to the service' do
+      it 'returns success' do
+        subject
+        result = subject
+
+        expect(result).to be_a(ServiceResponse)
+        expect(result.success?).to be(true)
+      end
+
+      it 'second call does not change the merge-ref' do
+        expect { subject }.to change(merge_request, :merge_ref_head).from(nil)
+        expect { subject }.not_to change(merge_request, :merge_ref_head)
+      end
+    end
+
     context 'when broken' do
       before do
         allow(merge_request).to receive(:broken?) { true }
@@ -61,6 +84,14 @@ describe MergeRequests::MergeabilityCheckService do
       end
 
       it_behaves_like 'unmergeable merge request'
+
+      it 'returns ServiceResponse.error' do
+        result = subject
+
+        expect(result).to be_a(ServiceResponse)
+        expect(result.error?).to be(true)
+        expect(result.message).to eq('Merge request is not mergeable')
+      end
     end
 
     context 'when it has conflicts' do
@@ -70,6 +101,14 @@ describe MergeRequests::MergeabilityCheckService do
       end
 
       it_behaves_like 'unmergeable merge request'
+
+      it 'returns ServiceResponse.error' do
+        result = subject
+
+        expect(result).to be_a(ServiceResponse)
+        expect(result.error?).to be(true)
+        expect(result.message).to eq('Merge request is not mergeable')
+      end
     end
 
     context 'when MR cannot be merged and has no merge ref' do
@@ -78,6 +117,14 @@ describe MergeRequests::MergeabilityCheckService do
       end
 
       it_behaves_like 'unmergeable merge request'
+
+      it 'returns ServiceResponse.error' do
+        result = subject
+
+        expect(result).to be_a(ServiceResponse)
+        expect(result.error?).to be(true)
+        expect(result.message).to eq('Merge request is not mergeable')
+      end
     end
 
     context 'when MR cannot be merged and has outdated merge ref' do
@@ -87,6 +134,54 @@ describe MergeRequests::MergeabilityCheckService do
       end
 
       it_behaves_like 'unmergeable merge request'
+
+      it 'returns ServiceResponse.error' do
+        result = subject
+
+        expect(result).to be_a(ServiceResponse)
+        expect(result.error?).to be(true)
+        expect(result.message).to eq('Merge request is not mergeable')
+      end
+    end
+
+    context 'when merge request is not given' do
+      subject { described_class.new(nil).execute }
+
+      it 'returns ServiceResponse.error' do
+        result = subject
+
+        expect(result).to be_a(ServiceResponse)
+        expect(result.message).to eq('Invalid argument')
+      end
+    end
+
+    context 'when read only DB' do
+      it 'returns ServiceResponse.error' do
+        allow(Gitlab::Database).to receive(:read_only?) { true }
+
+        result = subject
+
+        expect(result).to be_a(ServiceResponse)
+        expect(result.message).to eq('Unsupported operation')
+      end
+    end
+
+    context 'when MR is mergeable but merge-ref does not exists' do
+      before do
+        merge_request.mark_as_mergeable!
+      end
+
+      it 'keeps merge status as can_be_merged' do
+        expect { subject }.not_to change(merge_request, :merge_status).from('can_be_merged')
+      end
+
+      it 'returns ServiceResponse.error' do
+        result = subject
+
+        expect(result).to be_a(ServiceResponse)
+        expect(result.error?).to be(true)
+        expect(result.message).to eq('Merge ref was not found')
+      end
     end
   end
 end
diff --git a/spec/services/service_response_spec.rb b/spec/services/service_response_spec.rb
index 30bd4d6820b..e790d272e61 100644
--- a/spec/services/service_response_spec.rb
+++ b/spec/services/service_response_spec.rb
@@ -16,6 +16,13 @@ describe ServiceResponse do
       expect(response).to be_success
       expect(response.message).to eq('Good orange')
     end
+
+    it 'creates a successful response with payload' do
+      response = described_class.success(payload: { good: 'orange' })
+
+      expect(response).to be_success
+      expect(response.payload).to eq(good: 'orange')
+    end
   end
 
   describe '.error' do
@@ -33,6 +40,15 @@ describe ServiceResponse do
       expect(response.message).to eq('Bad apple')
       expect(response.http_status).to eq(400)
     end
+
+    it 'creates a failed response with payload' do
+      response = described_class.error(message: 'Bad apple',
+                                       payload: { bad: 'apple' })
+
+      expect(response).to be_error
+      expect(response.message).to eq('Bad apple')
+      expect(response.payload).to eq(bad: 'apple')
+    end
   end
 
   describe '#success?' do