Commit 96ff9c6b authored by Douwe Maan's avatar Douwe Maan

Merge branch 'osw-sync-merge-ref-upon-mergeability-check' into 'master'

Automatically update MR merge-ref along merge status

Closes #58495

See merge request gitlab-org/gitlab-ce!28513
parents a9589cdf 4246a621
......@@ -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
......@@ -251,10 +251,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 auto_merge_strategy is specified since we have
# to wait until CI completes to know
......
......@@ -725,19 +725,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)
if can_be_merged
mark_as_mergeable
else
mark_as_unmergeable
def check_mergeability
MergeRequests::MergeabilityCheckService.new(self).execute
end
# rubocop: enable CodeReuse/ServiceClass
# 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
......@@ -763,7 +760,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
......@@ -778,15 +775,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
......@@ -1114,6 +1102,12 @@ 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
def ref_path
"refs/#{Repository::REF_MERGE_REQUEST}/#{iid}/head"
end
......
......@@ -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
......
# frozen_string_literal: true
module MergeRequests
class MergeabilityCheckService < ::BaseService
include Gitlab::Utils::StrongMemoize
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(message: 'Invalid argument') unless merge_request
return ServiceResponse.error(message: '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
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?
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
# 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
---
title: Sync merge ref upon mergeability check
merge_request: 28513
author:
type: added
......@@ -1159,33 +1159,29 @@ Parameters:
}
```
## Merge to default merge ref path
## Returns the up to date merge-ref HEAD commit
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
{
......
......@@ -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
......
......@@ -397,28 +397,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 the up to date merge-ref HEAD commit'
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 = ::MergeRequests::MergeabilityCheckService.new(merge_request).execute
if result[:status] == :success
present result.slice(:commit_id), 200
if result.success?
present :commit_id, result.payload.dig(:merge_ref_head, :commit_id)
else
http_status = result[:http_status] || 400
render_api_error!(result[:message], http_status)
render_api_error!(result.message, 400)
end
end
......
......@@ -2012,57 +2012,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) }
......@@ -2076,7 +2025,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
......@@ -2090,7 +2039,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
......@@ -3158,38 +3107,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])
......
......@@ -1546,54 +1546,67 @@ 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' }
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 }
it 'returns 404' do
get api(url, user)
expect(response).to have_gitlab_http_status(404)
end
end
end
describe "PUT /projects/:id/merge_requests/:merge_request_iid" do
context 'updates force_remove_source_branch properly' do
......
......@@ -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
# 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
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
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 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 }
allow(project.repository).to receive(:can_be_merged?) { false }
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
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'
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
before do
merge_request.mark_as_unmergeable!
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
before do
MergeRequests::MergeToRefService.new(project, merge_request.author).execute(merge_request)
merge_request.mark_as_unmergeable!
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
......@@ -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
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment