From b965009ddddcd50e76841dbc97d2767292e88a0a Mon Sep 17 00:00:00 2001 From: Oswaldo Ferreira Date: Tue, 21 May 2019 18:14:22 -0300 Subject: Automatically update MR merge-ref along merge status This couples the code that transitions the `MergeRequest#merge_status` and refs/merge-requests/:iid/merge ref update. In general, instead of directly telling `MergeToRefService` to update the merge ref, we should rely on `MergeabilityCheckService` to keep both the merge status and merge ref synced. Now, if the merge_status is `can_be_merged` it means the merge-ref is also updated to the latest. We've also updated the logic to be more systematic and less user-based. --- .../projects/merge_requests_controller.rb | 6 +- app/models/merge_request.rb | 42 +++++----- .../merge_requests/merge_to_ref_service.rb | 20 +---- .../merge_requests/mergeability_check_service.rb | 54 +++++++++++++ .../osw-sync-merge-ref-upon-mergeability-check.yml | 5 ++ doc/api/merge_requests.md | 20 ++--- lib/api/entities.rb | 2 +- lib/api/merge_requests.rb | 24 ++---- spec/models/merge_request_spec.rb | 87 +------------------- spec/requests/api/merge_requests_spec.rb | 61 ++++++++------ .../merge_requests/merge_to_ref_service_spec.rb | 41 +++------- .../mergeability_check_service_spec.rb | 92 ++++++++++++++++++++++ 12 files changed, 239 insertions(+), 215 deletions(-) create mode 100644 app/services/merge_requests/mergeability_check_service.rb create mode 100644 changelogs/unreleased/osw-sync-merge-ref-upon-mergeability-check.yml create mode 100644 spec/services/merge_requests/mergeability_check_service_spec.rb diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 8f177895b08..7666ffaa3b5 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -33,7 +33,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo def show close_merge_request_if_no_source_project - mark_merge_request_mergeable + @merge_request.check_mergeability respond_to do |format| format.html do @@ -253,10 +253,6 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo @merge_request.has_no_commits? && !@merge_request.target_branch_exists? end - def mark_merge_request_mergeable - @merge_request.check_if_can_be_merged - end - def merge! # Disable the CI check if merge_when_pipeline_succeeds is enabled since we have # to wait until CI completes to know diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 311ba1ce6bd..25ad8f67496 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -712,19 +712,16 @@ class MergeRequest < ApplicationRecord MergeRequests::ReloadDiffsService.new(self, current_user).execute end - # rubocop: enable CodeReuse/ServiceClass - - def check_if_can_be_merged - return unless self.class.state_machines[:merge_status].check_state?(merge_status) && Gitlab::Database.read_write? - can_be_merged = - !broken? && project.repository.can_be_merged?(diff_head_sha, target_branch) + def check_mergeability + MergeRequests::MergeabilityCheckService.new(self).execute + end + # rubocop: enable CodeReuse/ServiceClass - if can_be_merged - mark_as_mergeable - else - mark_as_unmergeable - end + # Returns boolean indicating the merge_status should be rechecked in order to + # switch to either can_be_merged or cannot_be_merged. + def recheck_merge_status? + self.class.state_machines[:merge_status].check_state?(merge_status) end def merge_event @@ -750,7 +747,7 @@ class MergeRequest < ApplicationRecord def mergeable?(skip_ci_check: false) return false unless mergeable_state?(skip_ci_check: skip_ci_check) - check_if_can_be_merged + check_mergeability can_be_merged? && !should_be_rebased? end @@ -765,15 +762,6 @@ class MergeRequest < ApplicationRecord true end - def mergeable_to_ref? - return false unless mergeable_state?(skip_ci_check: true, skip_discussions_check: true) - - # Given the `merge_ref_path` will have the same - # state the `target_branch` would have. Ideally - # we need to check if it can be merged to it. - project.repository.can_be_merged?(diff_head_sha, target_branch) - end - def ff_merge_possible? project.repository.ancestor?(target_branch_sha, diff_head_sha) end @@ -1090,6 +1078,18 @@ class MergeRequest < ApplicationRecord target_project.repository.fetch_source_branch!(source_project.repository, source_branch, ref_path) end + # Returns the current merge-ref HEAD commit. + # + def merge_ref_head + project.repository.commit(merge_ref_path) + end + + # Returns the updated merge-ref HEAD commit. + # + def merge_ref_head! + merge_ref_head if check_mergeability.success? + end + def ref_path "refs/#{Repository::REF_MERGE_REQUEST}/#{iid}/head" end diff --git a/app/services/merge_requests/merge_to_ref_service.rb b/app/services/merge_requests/merge_to_ref_service.rb index 87147d90c32..8670b9ccf3d 100644 --- a/app/services/merge_requests/merge_to_ref_service.rb +++ b/app/services/merge_requests/merge_to_ref_service.rb @@ -20,20 +20,14 @@ module MergeRequests raise_error('Conflicts detected during merge') unless commit_id - commit = project.commit(commit_id) - target_id, source_id = commit.parent_ids - - success(commit_id: commit.id, - target_id: target_id, - source_id: source_id) - rescue MergeError => error + success(commit_id: commit_id) + rescue MergeError, ArgumentError => error error(error.message) end private def validate! - authorization_check! error_check! end @@ -43,21 +37,13 @@ module MergeRequests error = if !hooks_validation_pass?(merge_request) hooks_validation_error(merge_request) - elsif !@merge_request.mergeable_to_ref? - "Merge request is not mergeable to #{target_ref}" - elsif !source + elsif source.blank? 'No source for merge' end raise_error(error) if error end - def authorization_check! - unless Ability.allowed?(current_user, :admin_merge_request, project) - raise_error("You are not allowed to merge to this ref") - end - end - def target_ref merge_request.merge_ref_path end diff --git a/app/services/merge_requests/mergeability_check_service.rb b/app/services/merge_requests/mergeability_check_service.rb new file mode 100644 index 00000000000..d277d38127c --- /dev/null +++ b/app/services/merge_requests/mergeability_check_service.rb @@ -0,0 +1,54 @@ +# frozen_string_literal: true + +module MergeRequests + class MergeabilityCheckService < ::BaseService + delegate :project, to: :@merge_request + delegate :repository, to: :project + + def initialize(merge_request) + @merge_request = merge_request + end + + # Updates the MR merge_status. Whenever it switches to a can_be_merged state, + # the merge-ref is refreshed. + # + # Returns a ServiceResponse indicating merge_status is/became can_be_merged + # 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? + + update_merge_status + + unless merge_request.can_be_merged? + return ServiceResponse.error(message: 'Merge request is not mergeable') + end + + ServiceResponse.success + end + + private + + attr_reader :merge_request + + def update_merge_status + return unless merge_request.recheck_merge_status? + + if can_git_merge? + merge_to_ref && merge_request.mark_as_mergeable + else + merge_request.mark_as_unmergeable + end + end + + def can_git_merge? + !merge_request.broken? && repository.can_be_merged?(merge_request.diff_head_sha, merge_request.target_branch) + end + + def merge_to_ref + result = MergeRequests::MergeToRefService.new(project, merge_request.author).execute(merge_request) + result[:status] == :success + end + end +end diff --git a/changelogs/unreleased/osw-sync-merge-ref-upon-mergeability-check.yml b/changelogs/unreleased/osw-sync-merge-ref-upon-mergeability-check.yml new file mode 100644 index 00000000000..1f40089adb8 --- /dev/null +++ b/changelogs/unreleased/osw-sync-merge-ref-upon-mergeability-check.yml @@ -0,0 +1,5 @@ +--- +title: Sync merge ref upon mergeability check +merge_request: 28513 +author: +type: added diff --git a/doc/api/merge_requests.md b/doc/api/merge_requests.md index 7992af15448..08d57307279 100644 --- a/doc/api/merge_requests.md +++ b/doc/api/merge_requests.md @@ -1159,33 +1159,29 @@ Parameters: } ``` -## Merge to default merge ref path +## Return up to date merge-ref Merge the changes between the merge request source and target branches into `refs/merge-requests/:iid/merge` -ref, of the target project repository. This ref will have the state the target branch would have if +ref, of the target project repository, if possible. This ref will have the state the target branch would have if a regular merge action was taken. -This is not a regular merge action given it doesn't change the merge request state in any manner. +This is not a regular merge action given it doesn't change the merge request target branch state in any manner. -This ref (`refs/merge-requests/:iid/merge`) is **always** overwritten when submitting -requests to this API, so none of its state is kept or used in the process. +This ref (`refs/merge-requests/:iid/merge`) isn't necessarily overwritten when submitting +requests to this API, though it'll make sure the ref has the latest possible state. -If the merge request has conflicts, is empty or already merged, -you'll get a `400` and a descriptive error message. If you don't have permissions to do so, -you'll get a `403`. +If the merge request has conflicts, is empty or already merged, you'll get a `400` and a descriptive error message. -It returns the HEAD commit of `refs/merge-requests/:iid/merge` in the response body in -case of `200`. +It returns the HEAD commit of `refs/merge-requests/:iid/merge` in the response body in case of `200`. ``` -PUT /projects/:id/merge_requests/:merge_request_iid/merge_to_ref +GET /projects/:id/merge_requests/:merge_request_iid/merge_ref ``` Parameters: - `id` (required) - The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user - `merge_request_iid` (required) - Internal ID of MR -- `merge_commit_message` (optional) - Custom merge commit message ```json { diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 4e99fec02dd..b1b6e7bd7b9 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -699,7 +699,7 @@ module API # See https://gitlab.com/gitlab-org/gitlab-ce/issues/42344 for more # information. expose :merge_status do |merge_request| - merge_request.check_if_can_be_merged + merge_request.check_mergeability merge_request.merge_status end expose :diff_head_sha, as: :sha diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index ce85772e4ed..59b5346ffbb 100644 --- a/lib/api/merge_requests.rb +++ b/lib/api/merge_requests.rb @@ -398,28 +398,16 @@ module API present merge_request, with: Entities::MergeRequest, current_user: current_user, project: user_project end - desc 'Merge a merge request to its default temporary merge ref path' - params do - optional :merge_commit_message, type: String, desc: 'Custom merge commit message' - end - put ':id/merge_requests/:merge_request_iid/merge_to_ref' do + desc 'Returns an up to date merge-ref HEAD' + get ':id/merge_requests/:merge_request_iid/merge_ref' do merge_request = find_project_merge_request(params[:merge_request_iid]) - authorize! :admin_merge_request, user_project - - merge_params = { - commit_message: params[:merge_commit_message] - } - - result = ::MergeRequests::MergeToRefService - .new(merge_request.target_project, current_user, merge_params) - .execute(merge_request) + result = merge_request.check_mergeability - if result[:status] == :success - present result.slice(:commit_id), 200 + if result.success? && commit = merge_request.merge_ref_head + present :commit_id, commit.sha else - http_status = result[:http_status] || 400 - render_api_error!(result[:message], http_status) + render_api_error!(result.message, 400) end end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index c72b6e9033d..6e703023ed5 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -1962,57 +1962,6 @@ describe MergeRequest do end end - describe '#check_if_can_be_merged' do - let(:project) { create(:project, only_allow_merge_if_pipeline_succeeds: true) } - - shared_examples 'checking if can be merged' do - context 'when it is not broken and has no conflicts' do - before do - allow(subject).to receive(:broken?) { false } - allow(project.repository).to receive(:can_be_merged?).and_return(true) - end - - it 'is marked as mergeable' do - expect { subject.check_if_can_be_merged }.to change { subject.merge_status }.to('can_be_merged') - end - end - - context 'when broken' do - before do - allow(subject).to receive(:broken?) { true } - allow(project.repository).to receive(:can_be_merged?).and_return(false) - end - - it 'becomes unmergeable' do - expect { subject.check_if_can_be_merged }.to change { subject.merge_status }.to('cannot_be_merged') - end - end - - context 'when it has conflicts' do - before do - allow(subject).to receive(:broken?) { false } - allow(project.repository).to receive(:can_be_merged?).and_return(false) - end - - it 'becomes unmergeable' do - expect { subject.check_if_can_be_merged }.to change { subject.merge_status }.to('cannot_be_merged') - end - end - end - - context 'when merge_status is unchecked' do - subject { create(:merge_request, source_project: project, merge_status: :unchecked) } - - it_behaves_like 'checking if can be merged' - end - - context 'when merge_status is unchecked' do - subject { create(:merge_request, source_project: project, merge_status: :cannot_be_merged_recheck) } - - it_behaves_like 'checking if can be merged' - end - end - describe '#mergeable?' do let(:project) { create(:project) } @@ -2026,7 +1975,7 @@ describe MergeRequest do it 'return true if #mergeable_state? is true and the MR #can_be_merged? is true' do allow(subject).to receive(:mergeable_state?) { true } - expect(subject).to receive(:check_if_can_be_merged) + expect(subject).to receive(:check_mergeability) expect(subject).to receive(:can_be_merged?) { true } expect(subject.mergeable?).to be_truthy @@ -2040,7 +1989,7 @@ describe MergeRequest do it 'checks if merge request can be merged' do allow(subject).to receive(:mergeable_ci_state?) { true } - expect(subject).to receive(:check_if_can_be_merged) + expect(subject).to receive(:check_mergeability) subject.mergeable? end @@ -3108,38 +3057,6 @@ describe MergeRequest do end end - describe '#mergeable_to_ref?' do - it 'returns true when merge request is mergeable' do - subject = create(:merge_request) - - expect(subject.mergeable_to_ref?).to be(true) - end - - it 'returns false when merge request is already merged' do - subject = create(:merge_request, :merged) - - expect(subject.mergeable_to_ref?).to be(false) - end - - it 'returns false when merge request is closed' do - subject = create(:merge_request, :closed) - - expect(subject.mergeable_to_ref?).to be(false) - end - - it 'returns false when merge request is work in progress' do - subject = create(:merge_request, title: 'WIP: The feature') - - expect(subject.mergeable_to_ref?).to be(false) - end - - it 'returns false when merge request has no commits' do - subject = create(:merge_request, source_branch: 'empty-branch', target_branch: 'master') - - expect(subject.mergeable_to_ref?).to be(false) - end - end - describe '#merge_participants' do it 'contains author' do expect(subject.merge_participants).to eq([subject.author]) diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb index 5c94a87529b..c164c52c975 100644 --- a/spec/requests/api/merge_requests_spec.rb +++ b/spec/requests/api/merge_requests_spec.rb @@ -1546,52 +1546,65 @@ describe API::MergeRequests do end end - describe "PUT /projects/:id/merge_requests/:merge_request_iid/merge_to_ref" do - let(:pipeline) { create(:ci_pipeline_without_jobs) } + describe "GET /projects/:id/merge_requests/:merge_request_iid/merge_ref" do + before do + merge_request.mark_as_unchecked! + end + + let(:merge_request_iid) { merge_request.iid } + let(:url) do - "/projects/#{project.id}/merge_requests/#{merge_request.iid}/merge_to_ref" + "/projects/#{project.id}/merge_requests/#{merge_request_iid}/merge_ref" end it 'returns the generated ID from the merge service in case of success' do - put api(url, user), params: { merge_commit_message: 'Custom message' } - - commit = project.commit(json_response['commit_id']) + get api(url, user) expect(response).to have_gitlab_http_status(200) - expect(json_response['commit_id']).to be_present - expect(commit.message).to eq('Custom message') + expect(json_response['commit_id']).to eq(merge_request.merge_ref_head.sha) end it "returns 400 if branch can't be merged" do - merge_request.update!(state: 'merged') + merge_request.update!(merge_status: 'cannot_be_merged') - put api(url, user) + get api(url, user) expect(response).to have_gitlab_http_status(400) - expect(json_response['message']) - .to eq("Merge request is not mergeable to #{merge_request.merge_ref_path}") + expect(json_response['message']).to eq('Merge request is not mergeable') end - it 'returns 403 if user has no permissions to merge to the ref' do - user2 = create(:user) - project.add_reporter(user2) + context 'when user has no access to the MR' do + let(:project) { create(:project, :private) } + let(:merge_request) { create(:merge_request, source_project: project, target_project: project) } - put api(url, user2) + it 'returns 404' do + project.add_guest(user) - expect(response).to have_gitlab_http_status(403) - expect(json_response['message']).to eq('403 Forbidden') + get api(url, user) + + expect(response).to have_gitlab_http_status(404) + expect(json_response['message']).to eq('404 Not found') + end end - it 'returns 404 for an invalid merge request IID' do - put api("/projects/#{project.id}/merge_requests/12345/merge_to_ref", user) + context 'when invalid merge request IID' do + let(:merge_request_iid) { '12345' } - expect(response).to have_gitlab_http_status(404) + it 'returns 404' do + get api(url, user) + + expect(response).to have_gitlab_http_status(404) + end end - it "returns 404 if the merge request id is used instead of iid" do - put api("/projects/#{project.id}/merge_requests/#{merge_request.id}/merge", user) + context 'when merge request ID is used instead IID' do + let(:merge_request_iid) { merge_request.id } - expect(response).to have_gitlab_http_status(404) + it 'returns 404' do + get api(url, user) + + expect(response).to have_gitlab_http_status(404) + end end end diff --git a/spec/services/merge_requests/merge_to_ref_service_spec.rb b/spec/services/merge_requests/merge_to_ref_service_spec.rb index 0ac23050caf..5d492e4b013 100644 --- a/spec/services/merge_requests/merge_to_ref_service_spec.rb +++ b/spec/services/merge_requests/merge_to_ref_service_spec.rb @@ -32,10 +32,8 @@ describe MergeRequests::MergeToRefService do expect(result[:status]).to eq(:success) expect(result[:commit_id]).to be_present - expect(result[:source_id]).to eq(merge_request.source_branch_sha) - expect(result[:target_id]).to eq(merge_request.target_branch_sha) expect(repository.ref_exists?(target_ref)).to be(true) - expect(ref_head.id).to eq(result[:commit_id]) + expect(ref_head.sha).to eq(result[:commit_id]) end end @@ -72,10 +70,6 @@ describe MergeRequests::MergeToRefService do let(:merge_request) { create(:merge_request, :simple) } let(:project) { merge_request.project } - before do - project.add_maintainer(user) - end - describe '#execute' do let(:service) do described_class.new(project, user, commit_message: 'Awesome message', @@ -92,6 +86,12 @@ describe MergeRequests::MergeToRefService do it_behaves_like 'successfully evaluates pre-condition checks' context 'commit history comparison with regular MergeService' do + before do + # The merge service needs an authorized user while merge-to-ref + # doesn't. + project.add_maintainer(user) + end + let(:merge_ref_service) do described_class.new(project, user, {}) end @@ -136,9 +136,9 @@ describe MergeRequests::MergeToRefService do let(:merge_method) { :merge } it 'returns error' do - allow(merge_request).to receive(:mergeable_to_ref?) { false } + allow(project).to receive_message_chain(:repository, :merge_to_ref) { nil } - error_message = "Merge request is not mergeable to #{merge_request.merge_ref_path}" + error_message = 'Conflicts detected during merge' result = service.execute(merge_request) @@ -170,28 +170,5 @@ describe MergeRequests::MergeToRefService do it { expect(todo).not_to be_done } end - - context 'when merge request is WIP state' do - it 'fails to merge' do - merge_request = create(:merge_request, title: 'WIP: The feature') - - result = service.execute(merge_request) - - expect(result[:status]).to eq(:error) - expect(result[:message]).to eq("Merge request is not mergeable to #{merge_request.merge_ref_path}") - end - end - - it 'returns error when user has no authorization to admin the merge request' do - unauthorized_user = create(:user) - project.add_reporter(unauthorized_user) - - service = described_class.new(project, unauthorized_user) - - result = service.execute(merge_request) - - expect(result[:status]).to eq(:error) - expect(result[:message]).to eq('You are not allowed to merge to this ref') - end end end diff --git a/spec/services/merge_requests/mergeability_check_service_spec.rb b/spec/services/merge_requests/mergeability_check_service_spec.rb new file mode 100644 index 00000000000..6ad079d5f28 --- /dev/null +++ b/spec/services/merge_requests/mergeability_check_service_spec.rb @@ -0,0 +1,92 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe MergeRequests::MergeabilityCheckService do + shared_examples_for 'unmergeable merge request' do + it 'updates or keeps merge status as cannot_be_merged' do + subject + + expect(merge_request.merge_status).to eq('cannot_be_merged') + end + + it 'does not change the merge ref HEAD' do + expect { subject }.not_to change(merge_request, :merge_ref_head) + end + + it 'returns ServiceResponse.error' do + result = subject + + expect(result).to be_a(ServiceResponse) + expect(result).to be_error + end + end + + shared_examples_for 'mergeable merge request' do + it 'updates or keeps merge status as can_be_merged' do + subject + + expect(merge_request.merge_status).to eq('can_be_merged') + end + + it 'updates the merge ref' do + expect { subject }.to change(merge_request, :merge_ref_head).from(nil) + end + + it 'returns ServiceResponse.success' do + result = subject + + expect(result).to be_a(ServiceResponse) + expect(result).to be_success + end + end + + describe '#execute' do + let(:project) { create(:project, :repository) } + let(:merge_request) { create(:merge_request, merge_status: :unchecked, source_project: project, target_project: project) } + let(:repo) { project.repository } + + subject { described_class.new(merge_request).execute } + + before do + project.add_developer(merge_request.author) + end + + it_behaves_like 'mergeable merge request' + + context 'when broken' do + before do + allow(merge_request).to receive(:broken?) { true } + allow(project.repository).to receive(:can_be_merged?) { false } + end + + it_behaves_like 'unmergeable merge request' + end + + context 'when it has conflicts' do + before do + allow(merge_request).to receive(:broken?) { false } + allow(project.repository).to receive(:can_be_merged?) { false } + end + + it_behaves_like 'unmergeable merge request' + end + + context 'when MR cannot be merged and has no merge ref' do + before do + merge_request.mark_as_unmergeable! + end + + it_behaves_like 'unmergeable merge request' + end + + context 'when MR cannot be merged and has outdated merge ref' do + before do + MergeRequests::MergeToRefService.new(project, merge_request.author).execute(merge_request) + merge_request.mark_as_unmergeable! + end + + it_behaves_like 'unmergeable merge request' + end + end +end -- cgit v1.2.1