Skip to content

Migrate `WebHookWorker` to use `params` hash arg

About

Workers that accept a params argument make it much easier to add and remove new arguments to workers. Otherwise, adding or removing arguments is a muti-milestone effort.

!75821 (merged) introduced a params arg and #347389 (closed) is in the process of using it for the recursion detection UUID value. After that issue is complete, the signature of the .perform will be:

perform(hook_id, data, hook_name, params = {})

Proposal

We could additionally migrate hook_name to params also, making the final signature of .perform:

perform(hook_id, data, params = {})

With params containing hook_name and recursion_detection_uuid keys.

We need to keep data as a separate argument, as we use loggable_arguments to exclude that argument from being logged due to the possibility for sensitive information to be in the webhook payload. We possibly could move hook_id to params also - but it seems like a fundamental argument to accept, so it's not being proposed.

Timeframe

The timeframes given in the Deprecate and remove an argument docs for GitLab Sidekiq workers are to do with removing an argument in 3 milestones, but I think we can move an argument in 2:

%"14.12" or earlier

We bridge support for either:

diff --git a/app/services/web_hook_service.rb b/app/services/web_hook_service.rb
index a4342aa5b90..e7c219e6b97 100644
--- a/app/services/web_hook_service.rb
+++ b/app/services/web_hook_service.rb
@@ -103,10 +103,11 @@ def async_execute
       break log_recursion_blocked if recursion_blocked?

       params = {
+        hook_name: hook_name,
         recursion_detection_request_uuid: Gitlab::WebHooks::RecursionDetection::UUID.instance.request_uuid
       }.compact

-      WebHookWorker.perform_async(hook.id, data, hook_name, params)
+      WebHookWorker.perform_async(hook.id, data, nil, params)
     end
   end

diff --git a/app/workers/web_hook_worker.rb b/app/workers/web_hook_worker.rb
index fdcd22128a3..04b6c26ae86 100644
--- a/app/workers/web_hook_worker.rb
+++ b/app/workers/web_hook_worker.rb
@@ -16,13 +16,15 @@ class WebHookWorker
   # 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 = {})
+  def perform(hook_id, data, hook_name = nil, params = {})
     hook = WebHook.find_by_id(hook_id)
     return unless hook

     data = data.with_indifferent_access
     params.symbolize_keys!

+    hook_name ||= params.delete(:hook_name)
+
     # 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)

%15.0

We make a breaking change in a major release (as per our docs):


diff --git a/app/services/web_hook_service.rb b/app/services/web_hook_service.rb
index a4342aa5b90..e81fbae982e 100644
--- a/app/services/web_hook_service.rb
+++ b/app/services/web_hook_service.rb
@@ -103,10 +103,11 @@ def async_execute
       break log_recursion_blocked if recursion_blocked?

       params = {
         hook_name: hook_name,
         recursion_detection_request_uuid: Gitlab::WebHooks::RecursionDetection::UUID.instance.request_uuid
       }.compact

-      WebHookWorker.perform_async(hook.id, data, hook_name, params)
+      WebHookWorker.perform_async(hook.id, data, params)
     end
   end

diff --git a/app/workers/web_hook_worker.rb b/app/workers/web_hook_worker.rb
index fdcd22128a3..439b1573d9b 100644
--- a/app/workers/web_hook_worker.rb
+++ b/app/workers/web_hook_worker.rb
@@ -16,13 +16,15 @@ class WebHookWorker
   # 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 = {})
+  def perform(hook_id, data, params = {})
     hook = WebHook.find_by_id(hook_id)
     return unless hook

     data = data.with_indifferent_access
     params.symbolize_keys!

-    hook_name ||= params.delete(:hook_name)
+    hook_name = params.delete(:hook_name)

     # 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)
Edited by Luke Duncalfe