Enforce middleware ordering with a test, instead of comments

We want the actual order to be very predictable, so it's rightly defined
in code -- not with an on-the-fly tsort.

But we can do the tsort here, and then verify that it matches the
implemented ordering. This way we don't leave future readers guessing
which parts of the ordering are deliberate and which are arbitrary.

2 files changed, 35 insertions, 6 deletions

diff --git a/railties/lib/rails/application/default_middleware_stack.rb b/railties/lib/rails/application/default_middleware_stack.rb
index 5ecc7bb7c6..d070aca2dd 100644
--- a/railties/lib/rails/application/default_middleware_stack.rb
+++ b/railties/lib/rails/application/default_middleware_stack.rb
@@ -41,10 +41,8 @@ module Rails
           middleware.use ::Rack::Runtime
           middleware.use ::Rack::MethodOverride unless config.api_only
           middleware.use ::ActionDispatch::RequestId
-
           middleware.use ::ActionDispatch::RemoteIp, config.action_dispatch.ip_spoofing_check, config.action_dispatch.trusted_proxies
-          # Must come after Rack::MethodOverride to properly log overridden methods
-          # Must come after ActionDispatch::RemoteIP to properly log ip address
+
           middleware.use ::Rails::Rack::Logger, config.log_tags
           middleware.use ::ActionDispatch::ShowExceptions, show_exceptions_app
           middleware.use ::ActionDispatch::DebugExceptions, app, config.debug_exception_response_format
diff --git a/railties/test/application/middleware_test.rb b/railties/test/application/middleware_test.rb
index 71d822bb41..0a6e5b52e9 100644
--- a/railties/test/application/middleware_test.rb
+++ b/railties/test/application/middleware_test.rb
@@ -30,8 +30,8 @@ module ApplicationTests
         "Rack::Runtime",
         "Rack::MethodOverride",
         "ActionDispatch::RequestId",
-        "ActionDispatch::RemoteIp", # Must come before Rails::Rack::Logger to properly log request_id
-        "Rails::Rack::Logger", # must come after Rack::MethodOverride to properly log overridden methods
+        "ActionDispatch::RemoteIp",
+        "Rails::Rack::Logger",
         "ActionDispatch::ShowExceptions",
         "ActionDispatch::DebugExceptions",
         "ActionDispatch::Reloader",
@@ -59,7 +59,7 @@ module ApplicationTests
         "Rack::Runtime",
         "ActionDispatch::RequestId",
         "ActionDispatch::RemoteIp",
-        "Rails::Rack::Logger", # must come after Rack::MethodOverride to properly log overridden methods
+        "Rails::Rack::Logger",
         "ActionDispatch::ShowExceptions",
         "ActionDispatch::DebugExceptions",
         "ActionDispatch::Reloader",
@@ -70,6 +70,37 @@ module ApplicationTests
       ], middleware
     end
 
+    test "middleware dependencies" do
+      boot!
+
+      # The following array-of-arrays describes dependencies between
+      # middlewares: the first item in each list depends on the
+      # remaining items (and therefore must occur later in the
+      # middleware stack).
+
+      dependencies = [
+        # Logger needs a fully "corrected" request environment
+        %w(Rails::Rack::Logger Rack::MethodOverride ActionDispatch::RequestId ActionDispatch::RemoteIp),
+
+        # Serving public/ doesn't invoke user code, so it should skip
+        # locks etc
+        %w(ActionDispatch::Executor ActionDispatch::Static),
+
+        # Errors during reload must be reported
+        %w(ActionDispatch::Reloader ActionDispatch::ShowExceptions ActionDispatch::DebugExceptions),
+
+        # Outright dependencies
+        %w(ActionDispatch::Static Rack::Sendfile),
+        %w(ActionDispatch::Flash ActionDispatch::Session::CookieStore),
+        %w(ActionDispatch::Session::CookieStore ActionDispatch::Cookies),
+      ]
+
+      require "tsort"
+      sorted = TSort.tsort((middleware | dependencies.flatten).method(:each),
+                           lambda { |n, &b| dependencies.each { |m, *ds| ds.each(&b) if m == n } })
+      assert_equal sorted, middleware
+    end
+
     test "Rack::Cache is not included by default" do
       boot!