diff options
author | Robert Speicher <robert@gitlab.com> | 2017-12-22 17:18:22 +0000 |
---|---|---|
committer | Robert Speicher <robert@gitlab.com> | 2017-12-22 17:18:22 +0000 |
commit | 87dbf9846dcc870581ed753c730d768e671c51d4 (patch) | |
tree | c5fbc5bea4912bb1587720c55462702e9e786a58 | |
parent | 77d190b4b4ebd78daa965eb4a2ed4a1e3579d956 (diff) | |
parent | 16b8297e77501134cd93a74c3d5c60712930e7fd (diff) | |
download | gitlab-ce-87dbf9846dcc870581ed753c730d768e671c51d4.tar.gz |
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
-rw-r--r-- | app/models/project.rb | 16 | ||||
-rw-r--r-- | changelogs/unreleased/dm-issue-move-transaction-error.yml | 5 | ||||
-rw-r--r-- | config/initializers/forbid_sidekiq_in_transactions.rb | 8 | ||||
-rw-r--r-- | lib/after_commit_queue.rb | 10 | ||||
-rw-r--r-- | spec/services/issues/move_service_spec.rb | 12 |
5 files changed, 42 insertions, 9 deletions
diff --git a/app/models/project.rb b/app/models/project.rb index 3440c01b356..f9c640300ff 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -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 diff --git a/changelogs/unreleased/dm-issue-move-transaction-error.yml b/changelogs/unreleased/dm-issue-move-transaction-error.yml new file mode 100644 index 00000000000..0892169894a --- /dev/null +++ b/changelogs/unreleased/dm-issue-move-transaction-error.yml @@ -0,0 +1,5 @@ +--- +title: Execute project hooks and services after commit when moving an issue +merge_request: +author: +type: fixed diff --git a/config/initializers/forbid_sidekiq_in_transactions.rb b/config/initializers/forbid_sidekiq_in_transactions.rb index bedd57ede04..cb611aa21df 100644 --- a/config/initializers/forbid_sidekiq_in_transactions.rb +++ b/config/initializers/forbid_sidekiq_in_transactions.rb @@ -1,5 +1,7 @@ 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 diff --git a/lib/after_commit_queue.rb b/lib/after_commit_queue.rb index db63c5038ae..a4d8507960e 100644 --- a/lib/after_commit_queue.rb +++ b/lib/after_commit_queue.rb @@ -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 diff --git a/spec/services/issues/move_service_spec.rb b/spec/services/issues/move_service_spec.rb index f2b35a8fadf..2cad695d7c4 100644 --- a/spec/services/issues/move_service_spec.rb +++ b/spec/services/issues/move_service_spec.rb @@ -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 |