From bb9e1008525a9291014b6e394884ba74a9b7f9a4 Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Wed, 26 Jan 2022 13:49:28 +1300 Subject: [PATCH] Migrate WebHookWorker to use params: step 2 This change represents `Release M+1` in: https://gitlab.com/gitlab-org/gitlab/-/issues/347389 There should be no noticable changes. --- app/services/web_hook_service.rb | 6 ++++-- app/workers/web_hook_worker.rb | 16 +++++++++------- .../feature_flags/update_service_spec.rb | 2 +- spec/services/web_hook_service_spec.rb | 2 +- spec/workers/web_hook_worker_spec.rb | 11 ++++++++++- 5 files changed, 25 insertions(+), 12 deletions(-) diff --git a/app/services/web_hook_service.rb b/app/services/web_hook_service.rb index 9bfcd9a9ec6521..a4342aa5b90436 100644 --- a/app/services/web_hook_service.rb +++ b/app/services/web_hook_service.rb @@ -102,9 +102,11 @@ def async_execute break log_rate_limited if rate_limited? break log_recursion_blocked if recursion_blocked? - data[:_gitlab_recursion_detection_request_uuid] = Gitlab::WebHooks::RecursionDetection::UUID.instance.request_uuid + params = { + recursion_detection_request_uuid: Gitlab::WebHooks::RecursionDetection::UUID.instance.request_uuid + }.compact - WebHookWorker.perform_async(hook.id, data, hook_name) + WebHookWorker.perform_async(hook.id, data, hook_name, params) end end diff --git a/app/workers/web_hook_worker.rb b/app/workers/web_hook_worker.rb index 952ac94d5e6946..fdcd22128a35a4 100644 --- a/app/workers/web_hook_worker.rb +++ b/app/workers/web_hook_worker.rb @@ -6,14 +6,14 @@ class WebHookWorker include ApplicationWorker feature_category :integrations - loggable_arguments 2 + loggable_arguments 2, 3 data_consistency :delayed sidekiq_options retry: 4, dead: false urgency :low worker_has_external_dependencies! - # Webhook recursion detection properties are passed through the `data` arg. + # Webhook recursion detection properties may be passed through the `data` arg. # This will be migrated to the `params` arg over the next few releases. # See https://gitlab.com/gitlab-org/gitlab/-/issues/347389. def perform(hook_id, data, hook_name, params = {}) @@ -21,12 +21,14 @@ def perform(hook_id, data, hook_name, params = {}) return unless hook data = data.with_indifferent_access + params.symbolize_keys! - # Before executing the hook, reapply any recursion detection UUID that was - # initially present in the request header so the hook can pass this same header - # value in its request. - recursion_detection_uuid = data.delete(:_gitlab_recursion_detection_request_uuid) - Gitlab::WebHooks::RecursionDetection.set_request_uuid(recursion_detection_uuid) + # TODO: Remove in 14.9 https://gitlab.com/gitlab-org/gitlab/-/issues/347389 + params[:recursion_detection_request_uuid] ||= data.delete(:_gitlab_recursion_detection_request_uuid) + + # Before executing the hook, reapply any recursion detection UUID that was initially + # present in the request header so the hook can pass this same header value in its request. + Gitlab::WebHooks::RecursionDetection.set_request_uuid(params[:recursion_detection_request_uuid]) WebHookService.new(hook, data, hook_name, jid).execute end diff --git a/spec/services/feature_flags/update_service_spec.rb b/spec/services/feature_flags/update_service_spec.rb index abe0112b27ef78..f5e94c4af0fa3d 100644 --- a/spec/services/feature_flags/update_service_spec.rb +++ b/spec/services/feature_flags/update_service_spec.rb @@ -130,7 +130,7 @@ it 'executes hooks' do hook = create(:project_hook, :all_events_enabled, project: project) - expect(WebHookWorker).to receive(:perform_async).with(hook.id, an_instance_of(Hash), 'feature_flag_hooks') + expect(WebHookWorker).to receive(:perform_async).with(hook.id, an_instance_of(Hash), 'feature_flag_hooks', an_instance_of(Hash)) subject end diff --git a/spec/services/web_hook_service_spec.rb b/spec/services/web_hook_service_spec.rb index 14aa4e364a14ae..448d00076c1efc 100644 --- a/spec/services/web_hook_service_spec.rb +++ b/spec/services/web_hook_service_spec.rb @@ -404,7 +404,7 @@ def run_service describe '#async_execute' do def expect_to_perform_worker(hook) - expect(WebHookWorker).to receive(:perform_async).with(hook.id, data, 'push_hooks') + expect(WebHookWorker).to receive(:perform_async).with(hook.id, data, 'push_hooks', an_instance_of(Hash)) end def expect_to_rate_limit(hook, threshold:, throttled: false) diff --git a/spec/workers/web_hook_worker_spec.rb b/spec/workers/web_hook_worker_spec.rb index bbb8844a447ab8..dbdf7a2b978d5d 100644 --- a/spec/workers/web_hook_worker_spec.rb +++ b/spec/workers/web_hook_worker_spec.rb @@ -19,7 +19,16 @@ expect { subject.perform(non_existing_record_id, data, hook_name) }.not_to raise_error end - it 'retrieves recursion detection data, reinstates it, and cleans it from payload', :request_store, :aggregate_failures do + it 'retrieves recursion detection data and reinstates it', :request_store, :aggregate_failures do + uuid = SecureRandom.uuid + params = { recursion_detection_request_uuid: uuid } + + expect_next(WebHookService, project_hook, data.with_indifferent_access, hook_name, anything).to receive(:execute) + expect { subject.perform(project_hook.id, data, hook_name, params) } + .to change { Gitlab::WebHooks::RecursionDetection::UUID.instance.request_uuid }.to(uuid) + end + + it 'retrieves recursion detection data, reinstates it, and cleans it from payload when passed through as data', :request_store, :aggregate_failures do uuid = SecureRandom.uuid full_data = data.merge({ _gitlab_recursion_detection_request_uuid: uuid }) -- GitLab