Commit 16b8297e authored by Douwe Maan's avatar Douwe Maan

Execute project hooks and services after commit when moving an issue

parent 92e15071
......@@ -951,7 +951,9 @@ class Project < ActiveRecord::Base
def send_move_instructions(old_path_with_namespace)
# New project path needs to be committed to the DB or notification will
# 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
def owner
......@@ -963,15 +965,19 @@ class Project < ActiveRecord::Base
end
def execute_hooks(data, hooks_scope = :push_hooks)
hooks.public_send(hooks_scope).each do |hook| # rubocop:disable GitlabSecurity/PublicSend
hook.async_execute(data, hooks_scope.to_s)
run_after_commit_or_now do
hooks.public_send(hooks_scope).each do |hook| # rubocop:disable GitlabSecurity/PublicSend
hook.async_execute(data, hooks_scope.to_s)
end
end
end
def execute_services(data, hooks_scope = :push_hooks)
# Call only service hooks that are active for this scope
services.public_send(hooks_scope).each do |service| # rubocop:disable GitlabSecurity/PublicSend
service.async_execute(data)
run_after_commit_or_now do
services.public_send(hooks_scope).each do |service| # rubocop:disable GitlabSecurity/PublicSend
service.async_execute(data)
end
end
end
......
---
title: Execute project hooks and services after commit when moving an issue
merge_request:
author:
type: fixed
module Sidekiq
module Worker
EnqueueFromTransactionError = Class.new(StandardError)
mattr_accessor :skip_transaction_check
self.skip_transaction_check = false
......@@ -12,11 +14,11 @@ module Sidekiq
end
module ClassMethods
module NoSchedulingFromTransactions
module NoEnqueueingFromTransactions
%i(perform_async perform_at perform_in).each do |name|
define_method(name) do |*args|
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
race conditions when the worker runs before the transaction is committed and
tries to access a model that has not been saved yet.
......@@ -30,7 +32,7 @@ module Sidekiq
end
end
prepend NoSchedulingFromTransactions
prepend NoEnqueueingFromTransactions
end
end
end
......
......@@ -14,7 +14,15 @@ module AfterCommitQueue
def run_after_commit_or_now(&block)
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
instance_eval(&block)
end
......
......@@ -289,6 +289,18 @@ describe Issues::MoveService do
.to raise_error(StandardError, /Cannot move issue/)
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
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