Fix parameter reporting for Rack and Sinatra apps

The AbstractMiddleware tried to set the request parameters before the app handled the request. This could cause the request object to end up in a weird state where, if some other middleware (like a JSON payload parsing middleware) hadn't parsed the request payload yet, the parameters were reported as empty. Move the parameter setting to the "after" hook so that it doesn't try to read the parameters before they're parsed. Use the new `Transaction#set_params_if_nil` so that it doesn't overwrite any custom parameters set by the app. Fixes #1108
appsignal · Jun 21, 2024 · 0a253aa · 0a253aa
1 parent 9a20bf6
commit 0a253aa
Show file tree

Hide file tree

Showing 3 changed files with 7 additions and 4 deletions.
diff --git a/.changesets/fix-parameter-reporting-for-rack-and-sinatra-apps.md b/.changesets/fix-parameter-reporting-for-rack-and-sinatra-apps.md
@@ -0,0 +1,6 @@
+---
+bump: patch
+type: fix
+---
+
+Fix parameter reporting for Rack and Sinatra apps, especially POST payloads.
diff --git a/lib/appsignal/rack/abstract_middleware.rb b/lib/appsignal/rack/abstract_middleware.rb
@@ -91,8 +91,6 @@ def instrument_app_call(env)
       # Override this method to set metadata before the app is called.
       # Call `super` to also include the default set metadata.
       def add_transaction_metadata_before(transaction, request)
-        params = params_for(request)
-        transaction.params = params if params
       end
 
       # Add metadata to the transaction based on the request environment.
@@ -104,6 +102,7 @@ def add_transaction_metadata_after(transaction, request)
         transaction.set_action_if_nil(default_action)
         transaction.set_metadata("path", request.path)
         transaction.set_metadata("method", request.request_method)
+        transaction.set_params_if_nil(params_for(request))
         transaction.set_http_or_background_queue_start
       end
 

diff --git a/spec/lib/appsignal/transaction_spec.rb b/spec/lib/appsignal/transaction_spec.rb
@@ -402,8 +402,6 @@ def current_transaction
 
       context "when the params are not set" do
         it "sets the params on the transaction" do
-          expect(transaction.params).to be_nil
-
           params = { "key" => "value" }
           transaction.set_params_if_nil(params)