From 4246a62118d919e62b94d75eba641ed374c3f241 Mon Sep 17 00:00:00 2001 From: Oswaldo Ferreira Date: Fri, 31 May 2019 17:18:27 -0300 Subject: [PATCH] Add payload to the service response This introduces payload to the ServiceResponse with the merge ref HEAD commit data --- app/models/merge_request.rb | 4 - .../mergeability_check_service.rb | 34 ++++++- app/services/service_response.rb | 15 +-- lib/api/merge_requests.rb | 4 +- .../mergeability_check_service_spec.rb | 95 +++++++++++++++++++ spec/services/service_response_spec.rb | 16 ++++ 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 -- GitLab