Commit 87dbf984 authored by Robert Speicher's avatar Robert Speicher

Merge branch 'dm-issue-move-transaction-error' into 'master'

Execute project hooks and services after commit when moving an issue

Closes #41324

See merge request gitlab-org/gitlab-ce!16108
parents 77d190b4 16b8297e
...@@ -951,7 +951,9 @@ class Project < ActiveRecord::Base ...@@ -951,7 +951,9 @@ class Project < ActiveRecord::Base
def send_move_instructions(old_path_with_namespace) def send_move_instructions(old_path_with_namespace)
# New project path needs to be committed to the DB or notification will # New project path needs to be committed to the DB or notification will
# retrieve stale information # retrieve stale information
run_after_commit { NotificationService.new.project_was_moved(self, old_path_with_namespace) } run_after_commit do
NotificationService.new.project_was_moved(self, old_path_with_namespace)
end
end end
def owner def owner
...@@ -963,15 +965,19 @@ class Project < ActiveRecord::Base ...@@ -963,15 +965,19 @@ class Project < ActiveRecord::Base
end end
def execute_hooks(data, hooks_scope = :push_hooks) def execute_hooks(data, hooks_scope = :push_hooks)
hooks.public_send(hooks_scope).each do |hook| # rubocop:disable GitlabSecurity/PublicSend run_after_commit_or_now do
hook.async_execute(data, hooks_scope.to_s) hooks.public_send(hooks_scope).each do |hook| # rubocop:disable GitlabSecurity/PublicSend
hook.async_execute(data, hooks_scope.to_s)
end
end end
end end
def execute_services(data, hooks_scope = :push_hooks) def execute_services(data, hooks_scope = :push_hooks)
# Call only service hooks that are active for this scope # Call only service hooks that are active for this scope
services.public_send(hooks_scope).each do |service| # rubocop:disable GitlabSecurity/PublicSend run_after_commit_or_now do
service.async_execute(data) services.public_send(hooks_scope).each do |service| # rubocop:disable GitlabSecurity/PublicSend
service.async_execute(data)
end
end end
end end
......
---
title: Execute project hooks and services after commit when moving an issue
merge_request:
author:
type: fixed
module Sidekiq module Sidekiq
module Worker module Worker
EnqueueFromTransactionError = Class.new(StandardError)
mattr_accessor :skip_transaction_check mattr_accessor :skip_transaction_check
self.skip_transaction_check = false self.skip_transaction_check = false
...@@ -12,11 +14,11 @@ module Sidekiq ...@@ -12,11 +14,11 @@ module Sidekiq
end end
module ClassMethods module ClassMethods
module NoSchedulingFromTransactions module NoEnqueueingFromTransactions
%i(perform_async perform_at perform_in).each do |name| %i(perform_async perform_at perform_in).each do |name|
define_method(name) do |*args| define_method(name) do |*args|
if !Sidekiq::Worker.skip_transaction_check && AfterCommitQueue.inside_transaction? if !Sidekiq::Worker.skip_transaction_check && AfterCommitQueue.inside_transaction?
raise <<-MSG.strip_heredoc raise Sidekiq::Worker::EnqueueFromTransactionError, <<~MSG
`#{self}.#{name}` cannot be called inside a transaction as this can lead to `#{self}.#{name}` cannot be called inside a transaction as this can lead to
race conditions when the worker runs before the transaction is committed and race conditions when the worker runs before the transaction is committed and
tries to access a model that has not been saved yet. tries to access a model that has not been saved yet.
...@@ -30,7 +32,7 @@ module Sidekiq ...@@ -30,7 +32,7 @@ module Sidekiq
end end
end end
prepend NoSchedulingFromTransactions prepend NoEnqueueingFromTransactions
end end
end end
end end
......
...@@ -14,7 +14,15 @@ module AfterCommitQueue ...@@ -14,7 +14,15 @@ module AfterCommitQueue
def run_after_commit_or_now(&block) def run_after_commit_or_now(&block)
if AfterCommitQueue.inside_transaction? if AfterCommitQueue.inside_transaction?
run_after_commit(&block) if ActiveRecord::Base.connection.current_transaction.records.include?(self)
run_after_commit(&block)
else
# If the current transaction does not include this record, we can run
# the block now, even if it queues a Sidekiq job.
Sidekiq::Worker.skipping_transaction_check do
instance_eval(&block)
end
end
else else
instance_eval(&block) instance_eval(&block)
end end
......
...@@ -289,6 +289,18 @@ describe Issues::MoveService do ...@@ -289,6 +289,18 @@ describe Issues::MoveService do
.to raise_error(StandardError, /Cannot move issue/) .to raise_error(StandardError, /Cannot move issue/)
end end
end end
context 'project issue hooks' do
let(:hook) { create(:project_hook, project: old_project, issues_events: true) }
it 'executes project issue hooks' do
# Ideally, we'd test that `WebHookWorker.jobs.size` increased by 1,
# but since the entire spec run takes place in a transaction, we never
# actually get to the `after_commit` hook that queues these jobs.
expect { move_service.execute(old_issue, new_project) }
.not_to raise_error # Sidekiq::Worker::EnqueueFromTransactionError
end
end
end end
describe 'move permissions' do describe 'move permissions' 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