Commit c0d092b9 authored by Stan Hu's avatar Stan Hu

Merge branch '32737-make-mergeservice-idempotent' into 'master'

Make MergeService idempotent

Closes #32737

See merge request gitlab-org/gitlab!24708
parents 3ad22ba5 47825641
......@@ -27,6 +27,7 @@ module MergeRequests
success
end
end
log_info("Merge process finished on JID #{merge_jid} with state #{state}")
rescue MergeError => e
handle_merge_error(log_message: e.message, save_message_on_model: true)
......@@ -54,7 +55,7 @@ module MergeRequests
error =
if @merge_request.should_be_rebased?
'Only fast-forward merge is allowed for your project. Please update your source branch'
elsif !@merge_request.mergeable?
elsif !@merge_request.merged? && !@merge_request.mergeable?
'Merge request is not mergeable'
end
......
......@@ -8,17 +8,28 @@ module MergeRequests
#
class PostMergeService < MergeRequests::BaseService
def execute(merge_request)
merge_request.mark_as_merged
close_issues(merge_request)
todo_service.merge_merge_request(merge_request, current_user)
create_event(merge_request)
create_note(merge_request)
# These operations need to happen transactionally
ActiveRecord::Base.transaction(requires_new: true) do
merge_request.mark_as_merged
# These options do not call external services and should be
# relatively quick enough to put in a Transaction
create_event(merge_request)
todo_service.merge_merge_request(merge_request, current_user)
end
# These operations are idempotent so can be safely run multiple times
notification_service.merge_mr(merge_request, current_user)
execute_hooks(merge_request, 'merge')
create_note(merge_request)
close_issues(merge_request)
invalidate_cache_counts(merge_request, users: merge_request.assignees)
merge_request.update_project_counter_caches
delete_non_latest_diffs(merge_request)
cleanup_environments(merge_request)
# Anything after this point will be executed at-most-once. Less important activity only
# TODO: make all the work in here a separate sidekiq job so it can go in the transaction
execute_hooks(merge_request, 'merge')
end
private
......
......@@ -4,12 +4,14 @@ module MergeRequests
class SquashService < MergeRequests::BaseService
include Git::Logger
def idempotent?
true
end
def execute
# If performing a squash would result in no change, then
# immediately return a success message without performing a squash
if merge_request.commits_count < 2 && message.nil?
return success(squash_sha: merge_request.diff_head_sha)
end
return success(squash_sha: merge_request.diff_head_sha) if squash_redundant?
if merge_request.squash_in_progress?
return error(s_('MergeRequests|Squash task canceled: another squash is already in progress.'))
......@@ -20,6 +22,12 @@ module MergeRequests
private
def squash_redundant?
return true if merge_request.merged?
merge_request.commits_count < 2 && message.nil?
end
def squash!
squash_sha = repository.squash(current_user, merge_request, message || merge_request.default_squash_commit_message)
......
---
title: Make MergeService idempotent
merge_request: 24708
author:
type: changed
......@@ -47,6 +47,23 @@ describe MergeRequests::MergeService do
expect(note.note).to include 'merged'
end
it 'is idempotent' do
repository = project.repository
commit_count = repository.commit_count
merge_commit = merge_request.merge_commit.id
# a first invocation of execute is performed on the before block
service.execute(merge_request)
expect(merge_request.merge_error).to be_falsey
expect(merge_request).to be_valid
expect(merge_request).to be_merged
expect(repository.commits_by(oids: [merge_commit]).size).to eq(1)
expect(repository.commit_count).to eq(commit_count)
expect(merge_request.in_progress_merge_commit_sha).to be_nil
end
context 'when squashing' do
let(:merge_params) do
{ commit_message: 'Merge commit message',
......
......@@ -17,7 +17,6 @@ describe MergeRequests::PostMergeService do
it 'refreshes the number of open merge requests for a valid MR', :use_clean_rails_memory_store_caching do
# Cache the counter before the MR changed state.
project.open_merge_requests_count
merge_request.update!(state: 'merged')
service = described_class.new(project, user, {})
......
......@@ -137,6 +137,24 @@ describe MergeRequests::SquashService do
include_examples 'the squash succeeds'
end
context 'when the merge request has already been merged' do
let(:merge_request) { merge_request_with_one_commit }
it 'checks the side-effects for multiple calls' do
merge_request.mark_as_merged
expect(service).to be_idempotent
expect { IdempotentWorkerHelper::WORKER_EXEC_TIMES.times { service.execute } }.not_to raise_error
end
it 'idempotently returns a success' do
merge_request.mark_as_merged
result = service.execute
expect(result).to match(status: :success, squash_sha: merge_request.diff_head_sha)
end
end
context 'git errors' do
let(:merge_request) { merge_request_with_only_new_files }
let(:error) { 'A test error' }
......
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