Read about our upcoming Code of Conduct on this issue

Commit f51ebf7d authored by Douwe Maan's avatar Douwe Maan
Browse files

Execute project hooks and services after commit when moving an issue

parent 6b2c34e0a3d9
......@@ -951,7 +951,9 @@ def items_for(entity)
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 @@ def owner
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 @@ def self.skipping_transaction_check(&block)
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 NoSchedulingFromTransactions
end
end
prepend NoSchedulingFromTransactions
prepend NoEnqueueingFromTransactions
end
end
end
......
......@@ -14,7 +14,15 @@ def run_after_commit(&block)
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 @@
.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