Skip to content

Commit 7c99813

Browse files
committed
Don't rebuild the middleware stack when using with_routing:
- Fix rails#54595 - ### Problem When using the `with_routing` helper method in an integration test, we rebuild the middleware stack so that the entrypoint of our new stack is the new RouteSet object. By rebuilding the middleware stack from scratch, we may end un-configuring a middleware from an application. We also break the contract of the Rails initialization process. For instance, if you have a middleware that needs to be configured, after the Rails routes are loaded, this is what it would look like: ```ruby # config/application.rb warden_config = nil config.middleware.use Warden::Manager do |config| # The Warden middleware yields its config when the middleware is # instantiated. We can't configure warden yet as the routes # are not yet loaded. warden_config = config end initializer :configure_warden do ActiveSupport.on_load(:after_routes_loaded) do # The routes are now loaded, we can configure warden end end ``` This snippet follows the order of the documented Rails initialization process where first the middleware stack is built and then the routes gets loaded. -------------------- By rebuilding the middleware stack, we unconfigure what was done in the initializer. ### Solution Since rebuilding the middleware stack has some caveats, my idea is to modify the middleware stack original entry point and redefine `call` so that it calls our new RouteSet middleware. We have a reference to the original RouteSet middleware thanks to the `application.routes` method.
1 parent c218fb6 commit 7c99813

File tree

2 files changed

+63
-8
lines changed

2 files changed

+63
-8
lines changed

actionpack/lib/action_dispatch/testing/assertions/routing.rb

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
require "uri"
66
require "active_support/core_ext/hash/indifferent_access"
77
require "active_support/core_ext/string/access"
8+
require "active_support/core_ext/module/redefine_method"
89
require "action_controller/metal/exceptions"
910

1011
module ActionDispatch
@@ -20,38 +21,43 @@ module WithIntegrationRouting # :nodoc:
2021
module ClassMethods
2122
def with_routing(&block)
2223
old_routes = nil
24+
old_routes_call_method = nil
2325
old_integration_session = nil
2426

2527
setup do
2628
old_routes = app.routes
29+
old_routes_call_method = old_routes.method(:call)
2730
old_integration_session = integration_session
2831
create_routes(&block)
2932
end
3033

3134
teardown do
32-
reset_routes(old_routes, old_integration_session)
35+
reset_routes(old_routes, old_routes_call_method, old_integration_session)
3336
end
3437
end
3538
end
3639

3740
def with_routing(&block)
3841
old_routes = app.routes
42+
old_routes_call_method = old_routes.method(:call)
3943
old_integration_session = integration_session
4044
create_routes(&block)
4145
ensure
42-
reset_routes(old_routes, old_integration_session)
46+
reset_routes(old_routes, old_routes_call_method, old_integration_session)
4347
end
4448

4549
private
4650
def create_routes
4751
app = self.app
4852
routes = ActionDispatch::Routing::RouteSet.new
49-
rack_app = app.config.middleware.build(routes)
53+
54+
@original_routes ||= app.routes
55+
@original_routes.singleton_class.redefine_method(:call, &routes.method(:call))
56+
5057
https = integration_session.https?
5158
host = integration_session.host
5259

5360
app.instance_variable_set(:@routes, routes)
54-
app.instance_variable_set(:@app, rack_app)
5561
@integration_session = Class.new(ActionDispatch::Integration::Session) do
5662
include app.routes.url_helpers
5763
include app.routes.mounted_helpers
@@ -63,11 +69,9 @@ def create_routes
6369
yield routes
6470
end
6571

66-
def reset_routes(old_routes, old_integration_session)
67-
old_rack_app = app.config.middleware.build(old_routes)
68-
72+
def reset_routes(old_routes, old_routes_call_method, old_integration_session)
6973
app.instance_variable_set(:@routes, old_routes)
70-
app.instance_variable_set(:@app, old_rack_app)
74+
@original_routes.singleton_class.redefine_method(:call, &old_routes_call_method)
7175
@integration_session = old_integration_session
7276
@routes = old_routes
7377
end

actionpack/test/dispatch/routing_assertions_test.rb

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -351,3 +351,54 @@ class WithRoutingSettingsTest < ActionDispatch::IntegrationTest
351351
end
352352
end
353353
end
354+
355+
class WithRoutingResetTest < ActionDispatch::IntegrationTest
356+
test "with_routing doesn't rebuild the middleware stack" do
357+
middleware = Class.new do
358+
def initialize(app)
359+
@app = app
360+
@config = {}
361+
362+
yield(@config)
363+
end
364+
365+
def call(env)
366+
[200, { Rack::CONTENT_TYPE => "text/plain; charset=UTF-8" }, [@config[:foo]]]
367+
end
368+
end
369+
370+
middleware_config = nil
371+
372+
@app = self.class.build_app do |middleware_stack|
373+
middleware_stack.use middleware do |config|
374+
middleware_config = config
375+
end
376+
end
377+
@app.routes.draw do
378+
get("/purchase", to: "store#purchase")
379+
end
380+
381+
middleware_config[:foo] = "bar"
382+
383+
# Ensure the middleware is properly configured before calling `with_routing`
384+
get "/purchase"
385+
assert_response(:ok)
386+
assert_equal("bar", response.body)
387+
388+
with_routing do |route_set|
389+
route_set.draw do
390+
get "/purchase", to: "store#purchase"
391+
end
392+
393+
# Ensure the middleware is properly configured inside `with_routing`
394+
get "/purchase"
395+
assert_response(:ok)
396+
assert_equal("bar", response.body)
397+
end
398+
399+
# Ensure the middleware is properly configured after `with_routing`
400+
get "/purchase"
401+
assert_response(:ok)
402+
assert_equal("bar", response.body)
403+
end
404+
end

0 commit comments

Comments
 (0)