Prevent ActionDispatch::ServerTiming from overwriting existing header value

[jtmalinowski]

Summary

Fixes #45607

Tiny fix, if a middleware adds anything to the Server-Timing header, and is down the chain from ActionDispatch::ServerTiming, the value it set will be overwritten, this PR changes this behaviour, so that ActionDispatch::ServerTiming appends its entries instead.

[jtmalinowski]

Done, and, not sure if there can ever be no ActiveSupport Notification events in a response, but I changed the code slightly to handle that too.

[jonathanhefner]

Let's keep this name.

Suggested change

	our_server_timings = events.group_by(&:name).map do |event_name, events_collection|
	header_info = events.group_by(&:name).map do |event_name, events_collection|

[jonathanhefner]

This seems to be doing a bit more work than before. Would the following also work?

Suggested change

	headers[SERVER_TIMING_HEADER] =
	[headers[SERVER_TIMING_HEADER]].concat(our_server_timings).map(&:to_s).map(&:strip).reject(&:empty?).join(", ")
	header_info.prepend(headers[SERVER_TIMING_HEADER]) if headers[SERVER_TIMING_HEADER].present?
	headers[SERVER_TIMING_HEADER] = header_info.join(", ")

[jonathanhefner]

%w() creates an array, whereas %() quotes a string.

Suggested change

	headers["Server-Timing"] = [headers["Server-Timing"], %w(entry;desc="description")].compact.join(", ")
	headers["Server-Timing"] = [headers["Server-Timing"], %(entry;desc="description")].compact.join(", ")

[jonathanhefner]

This kwarg feels somewhat out of place to me. What do you think about setting up a @middlewares array (which matches @app), and appending to that?

index bb581e51e0..ada355be38 100644
--- a/actionpack/test/dispatch/server_timing_test.rb
+++ b/actionpack/test/dispatch/server_timing_test.rb
@@ -22,6 +22,10 @@ def create
     end
   end
 
+  setup do
+    @middlewares = [ActionDispatch::ServerTiming]
+  end
+
   test "server timing header is included in the response" do
     with_test_route_set do
       get "/"
@@ -45,7 +49,7 @@ def create
   end
 
   test "does not overwrite existing header values" do
-    class DummyServerTiming
+    @middlewares << Class.new do
       def initialize(app)
         @app = app
       end
@@ -57,7 +61,7 @@ def call(env)
       end
     end
 
-    with_test_route_set(later_middlewares: [DummyServerTiming]) do
+    with_test_route_set do
       get "/"
       assert_match(/entry;desc="description"/, @response.headers["Server-Timing"])
       assert_match(/start_processing.action_controller;dur=\w+/, @response.headers["Server-Timing"])
@@ -72,7 +76,7 @@ def call(env)
   end
 
   private
-    def with_test_route_set(later_middlewares: [])
+    def with_test_route_set
       with_routing do |set|
         set.draw do
           get "/", to: ::ServerTimingTest::TestController.action(:index)
@@ -81,8 +85,7 @@ def with_test_route_set(later_middlewares: [])
         end
 
         @app = self.class.build_app(set) do |middleware|
-          middleware.use ActionDispatch::ServerTiming
-          later_middlewares.each(&middleware.method(:use))
+          @middlewares.each { |m| middleware.use m }
         end
 
         yield

[jtmalinowski]

@jonathanhefner All the suggestions were great, thanks for reviewing.

[jonathanhefner]

Thank you, @jtmalinowski! 😃

(Backported to 7-0-stable.)