diff --git a/app/services/merge_requests/base_service.rb b/app/services/merge_requests/base_service.rb index 02e36cb544a035195d0114ec8f2e5e8e18af4d09..97e0e83806a6fb35b435613e28cf05b9866292cf 100644 --- a/app/services/merge_requests/base_service.rb +++ b/app/services/merge_requests/base_service.rb @@ -330,6 +330,18 @@ def duo_code_review_bot def invalidate_all_users_cache_count(merge_request) invalidate_cache_counts(merge_request, users: [*merge_request.assignees, *merge_request.reviewers, merge_request.author].uniq) end + + override :change_state + def change_state(merge_request) + case params[:state_event] + when 'close' + close_service.new(**close_service.constructor_container_arg(project), current_user: current_user) + .execute(merge_request, skip_reset: true) + params.delete(:state_event) + else + super + end + end end end diff --git a/app/services/merge_requests/close_service.rb b/app/services/merge_requests/close_service.rb index b9d2098eba395be6c9ce78d3f17e3f68c276bb75..6707eaff17ae9841b3c07c8c7057552d200f5e32 100644 --- a/app/services/merge_requests/close_service.rb +++ b/app/services/merge_requests/close_service.rb @@ -4,7 +4,7 @@ module MergeRequests class CloseService < MergeRequests::BaseService include RemovesRefs - def execute(merge_request, commit = nil) + def execute(merge_request, commit = nil, skip_reset: false) if Feature.enabled?(:destroy_fork_network_on_archive, project) unless params[:skip_authorization].present? || can?(current_user, :update_merge_request, merge_request) return merge_request @@ -13,32 +13,42 @@ def execute(merge_request, commit = nil) return merge_request unless can?(current_user, :update_merge_request, merge_request) end - # If we close MergeRequest we want to ignore validation - # so we can close broken one (Ex. fork project removed) - merge_request.allow_broken = true - - if merge_request.close - expire_unapproved_key(merge_request) - create_event(merge_request) - merge_request_activity_counter.track_close_mr_action(user: current_user) - create_note(merge_request) - notification_service.async.close_mr(merge_request, current_user) - todo_service.close_merge_request(merge_request, current_user) - execute_hooks(merge_request, 'close') - invalidate_all_users_cache_count(merge_request) - merge_request.invalidate_project_counter_caches - cleanup_environments(merge_request) - deactivate_pages_deployments(merge_request) - abort_auto_merge(merge_request, 'merge request was closed') - cleanup_refs(merge_request) - trigger_merge_request_merge_status_updated(merge_request) - end + use_primary(merge_request, skip_reset) do |merge_request| + # If we close MergeRequest we want to ignore validation + # so we can close broken one (Ex. fork project removed) + merge_request.allow_broken = true + + if merge_request.close + expire_unapproved_key(merge_request) + create_event(merge_request) + merge_request_activity_counter.track_close_mr_action(user: current_user) + create_note(merge_request) + notification_service.async.close_mr(merge_request, current_user) + todo_service.close_merge_request(merge_request, current_user) + execute_hooks(merge_request, 'close') + invalidate_all_users_cache_count(merge_request) + merge_request.invalidate_project_counter_caches + cleanup_environments(merge_request) + deactivate_pages_deployments(merge_request) + abort_auto_merge(merge_request, 'merge request was closed') + cleanup_refs(merge_request) + trigger_merge_request_merge_status_updated(merge_request) + end - merge_request + merge_request + end end private + def use_primary(merge_request, skip_reset) + return yield(merge_request) if skip_reset + + ::Gitlab::Database::LoadBalancing::SessionMap.current(merge_request.load_balancer).use_primary do + yield(merge_request.reset) + end + end + def create_event(merge_request) # Making sure MergeRequest::Metrics updates are in sync with # Event creation. diff --git a/ee/app/services/ee/merge_requests/close_service.rb b/ee/app/services/ee/merge_requests/close_service.rb index 2d47a9d047144f74a517222d488a6928d2264e87..f8edf726d5d41316e348bab3823235935c048d3f 100644 --- a/ee/app/services/ee/merge_requests/close_service.rb +++ b/ee/app/services/ee/merge_requests/close_service.rb @@ -6,7 +6,7 @@ module CloseService extend ::Gitlab::Utils::Override override :execute - def execute(merge_request, commit = nil) + def execute(merge_request, commit = nil, skip_reset: false) super.tap do if current_user.project_bot? log_audit_event(merge_request, 'merge_request_closed_by_project_bot', diff --git a/spec/services/merge_requests/close_service_spec.rb b/spec/services/merge_requests/close_service_spec.rb index 55c9b3ff1d0b90d24d793de988d08163354f75ed..a524ef79be85c018b22211c04f81190cf5cdaca6 100644 --- a/spec/services/merge_requests/close_service_spec.rb +++ b/spec/services/merge_requests/close_service_spec.rb @@ -66,6 +66,23 @@ def execute end end + context 'when the merge request is merged concurrently in another process' do + it 'does not close the merge request' do + stale_instance = MergeRequest.find(merge_request.id) + fresh_instance = MergeRequest.find(merge_request.id) + + fresh_instance.mark_as_merged! + + result = service.execute(stale_instance) + + expect(result).to eq(stale_instance) + expect(result).to be_merged + expect(result).not_to be_closed + + expect(merge_request.reload).to be_merged + end + end + it 'updates metrics' do metrics = merge_request.metrics metrics_service = double(MergeRequestMetricsService) diff --git a/spec/support/shared_examples/services/issuable_shared_examples.rb b/spec/support/shared_examples/services/issuable_shared_examples.rb index 2357dabf350f8eb83abd3b395304b095577df4c0..97c78a08d917e798d630cca49028e01ca8ea19c4 100644 --- a/spec/support/shared_examples/services/issuable_shared_examples.rb +++ b/spec/support/shared_examples/services/issuable_shared_examples.rb @@ -2,13 +2,19 @@ RSpec.shared_examples 'cache counters invalidator' do it 'invalidates counter cache for assignees' do - expect(merge_request.assignees).to all(receive(:invalidate_merge_request_cache_counts)) + assignees = merge_request.assignees.to_a + allow(merge_request).to receive(:assignees).and_return(assignees) + + expect(assignees).to all(receive(:invalidate_merge_request_cache_counts)) described_class.new(project: project, current_user: user).execute(merge_request) end it 'invalidates counter cache for author' do - expect(merge_request.author).to receive(:invalidate_merge_request_cache_counts) + author = merge_request.author + allow(merge_request).to receive(:author).and_return(author) + + expect(author).to receive(:invalidate_merge_request_cache_counts) described_class.new(project: project, current_user: user).execute(merge_request) end diff --git a/spec/support/shared_examples/services/merge_request_shared_examples.rb b/spec/support/shared_examples/services/merge_request_shared_examples.rb index 76526f1a99fe142dc888985161cffae89b5c4e12..146100d40ff5a335312a81e05ad9af325033bd8a 100644 --- a/spec/support/shared_examples/services/merge_request_shared_examples.rb +++ b/spec/support/shared_examples/services/merge_request_shared_examples.rb @@ -52,7 +52,10 @@ end it 'invalidates counter cache for reviewers' do - expect(merge_request.reviewers).to all(receive(:invalidate_merge_request_cache_counts)) + reviewers = merge_request.reviewers.to_a + allow(merge_request).to receive(:reviewers).and_return(reviewers) + + expect(reviewers).to all(receive(:invalidate_merge_request_cache_counts)) described_class.new(project: project, current_user: user).execute(merge_request) end