Skip to content

Commit

Permalink
Defer route drawing to the first request, or when url_helpers called.
Browse files Browse the repository at this point in the history
Executes the first routes reload in middleware, or when the route set
url_helpers is called. Previously, this was executed unconditionally on
boot, which can slow down boot time unnecessarily for larger apps with
lots of routes.
  • Loading branch information
gmcgibbon committed Jun 4, 2024
1 parent 060d6ca commit 7ac3338
Show file tree
Hide file tree
Showing 24 changed files with 374 additions and 47 deletions.
4 changes: 3 additions & 1 deletion actionpack/lib/action_dispatch/testing/assertions/routing.rb
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,9 @@ def with_routing(&block)
def create_routes
app = self.app
routes = ActionDispatch::Routing::RouteSet.new
rack_app = app.config.middleware.build(routes)
middleware = app.config.middleware.dup
middleware.delete(Rails::Rack::LoadRoutes) if defined?(Rails::Rack::LoadRoutes)
rack_app = middleware.build(routes)
https = integration_session.https?
host = integration_session.host

Expand Down
2 changes: 2 additions & 0 deletions actionpack/test/abstract_unit.rb
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ def env
@_env ||= ActiveSupport::StringInquirer.new(ENV["RAILS_ENV"] || ENV["RACK_ENV"] || "test")
end

def application; end

def root; end
end
end
Expand Down
32 changes: 31 additions & 1 deletion actionpack/test/dispatch/routing_assertions_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,11 @@
require "rails/engine"
require "controller/fake_controllers"

class SecureArticlesController < ArticlesController; end
class SecureArticlesController < ArticlesController
def index
render(inline: "")
end
end
class BlockArticlesController < ArticlesController; end
class QueryArticlesController < ArticlesController; end

Expand Down Expand Up @@ -276,6 +280,20 @@ class RoutingAssertionsControllerTest < ActionController::TestCase

class WithRoutingTest < ActionController::TestCase
include RoutingAssertionsSharedTests::WithRoutingSharedTests

test "with_routing routes are reachable" do
@controller = SecureArticlesController.new

with_routing do |routes|
routes.draw do
get :new_route, to: "secure_articles#index"
end

get :index

assert_predicate(response, :ok?)
end
end
end
end

Expand All @@ -295,6 +313,18 @@ class RoutingAssertionsIntegrationTest < ActionDispatch::IntegrationTest

class WithRoutingTest < ActionDispatch::IntegrationTest
include RoutingAssertionsSharedTests::WithRoutingSharedTests

test "with_routing routes are reachable" do
with_routing do |routes|
routes.draw do
get :new_route, to: "secure_articles#index"
end

get "/new_route"

assert_predicate(response, :ok?)
end
end
end

class WithRoutingSettingsTest < ActionDispatch::IntegrationTest
Expand Down
9 changes: 9 additions & 0 deletions railties/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,12 @@
* Defer route drawing to the first request, or when url_helpers are called

Executes the first routes reload in middleware, or when a route set's
url_helpers receives a route call / asked if it responds to a route.
Previously, this was executed unconditionally on boot, which can
slow down boot time unnecessarily for larger apps with lots of routes.

*Gannon McGibbon*

* Add Rubocop and GitHub Actions to plugin generator.
This can be skipped using --skip-rubocop and --skip-ci.

Expand Down
5 changes: 5 additions & 0 deletions railties/lib/rails/application.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
require "active_support/encrypted_configuration"
require "active_support/hash_with_indifferent_access"
require "active_support/configuration_file"
require "active_support/parameter_filter"
require "rails/engine"
require "rails/autoloaders"

Expand Down Expand Up @@ -153,6 +154,10 @@ def reload_routes!
routes_reloader.reload!
end

def reload_routes_unless_loaded # :nodoc:
initialized? && routes_reloader.execute_unless_loaded
end

# Returns a key generator (ActiveSupport::CachingKeyGenerator) for a
# specified +secret_key_base+. The return value is memoized, so additional
# calls with the same +secret_key_base+ will return the same key generator
Expand Down
2 changes: 2 additions & 0 deletions railties/lib/rails/application/default_middleware_stack.rb
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ def build_stack
middleware.use ::ActionDispatch::ActionableExceptions
end

middleware.use ::Rails::Rack::LoadRoutes unless app.config.eager_load

if config.reloading_enabled?
middleware.use ::ActionDispatch::Reloader, app.reloader
end
Expand Down
8 changes: 6 additions & 2 deletions railties/lib/rails/application/finisher.rb
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,6 @@ def self.complete(_state)
initializer :set_routes_reloader_hook do |app|
reloader = routes_reloader
reloader.eager_load = app.config.eager_load
reloader.execute
reloaders << reloader

app.reloader.to_run do
Expand All @@ -177,7 +176,12 @@ def self.complete(_state)
ActiveSupport.run_load_hooks(:after_routes_loaded, self)
end

ActiveSupport.run_load_hooks(:after_routes_loaded, self)
if reloader.eager_load
reloader.execute
ActiveSupport.run_load_hooks(:after_routes_loaded, self)
elsif reloader.loaded
ActiveSupport.run_load_hooks(:after_routes_loaded, self)
end
end

# Set clearing dependencies after the finisher hook to ensure paths
Expand Down
12 changes: 11 additions & 1 deletion railties/lib/rails/application/routes_reloader.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ class Application
class RoutesReloader
include ActiveSupport::Callbacks

attr_reader :route_sets, :paths, :external_routes
attr_reader :route_sets, :paths, :external_routes, :loaded
attr_accessor :eager_load
attr_writer :run_after_load_paths # :nodoc:
delegate :execute_if_updated, :execute, :updated?, to: :updater
Expand All @@ -17,9 +17,11 @@ def initialize
@route_sets = []
@external_routes = []
@eager_load = false
@loaded = false
end

def reload!
@loaded = true
clear!
load_paths
finalize!
Expand All @@ -28,6 +30,14 @@ def reload!
revert
end

def execute_unless_loaded
unless @loaded
execute
ActiveSupport.run_load_hooks(:after_routes_loaded, Rails.application)
true
end
end

private
def updater
@updater ||= begin
Expand Down
1 change: 1 addition & 0 deletions railties/lib/rails/commands/routes/routes_command.rb
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ def invoke_command(*)
desc "routes", "List all the defined routes"
def perform(*)
boot_application!
Rails.application.reload_routes_unless_loaded
require "action_dispatch/routing/inspector"

say inspector.format(formatter, routes_filter)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ def action_missing?

def perform(*)
boot_application!
Rails.application.reload_routes_unless_loaded
require "action_dispatch/routing/inspector"

say(inspector.format(formatter, routes_filter))
Expand Down
3 changes: 2 additions & 1 deletion railties/lib/rails/engine.rb
Original file line number Diff line number Diff line change
Expand Up @@ -349,6 +349,7 @@ module Rails
# config.railties_order = [Blog::Engine, :main_app, :all]
class Engine < Railtie
autoload :Configuration, "rails/engine/configuration"
autoload :RouteSet, "rails/engine/route_set"

class << self
attr_accessor :called_from, :isolated
Expand Down Expand Up @@ -543,7 +544,7 @@ def env_config
# Defines the routes for this engine. If a block is given to
# routes, it is appended to the engine.
def routes(&block)
@routes ||= ActionDispatch::Routing::RouteSet.new_with_config(config)
@routes ||= RouteSet.new_with_config(config)
@routes.append(&block) if block_given?
@routes
end
Expand Down
54 changes: 54 additions & 0 deletions railties/lib/rails/engine/route_set.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
# frozen_string_literal: true

# :markup: markdown

require "action_dispatch/routing/route_set"

module Rails
class Engine
class RouteSet < ActionDispatch::Routing::RouteSet # :nodoc:
class NamedRouteCollection < ActionDispatch::Routing::RouteSet::NamedRouteCollection
def route_defined?(name)
Rails.application&.reload_routes_unless_loaded

super(name)
end
end

def initialize(config = DEFAULT_CONFIG)
super
self.named_routes = NamedRouteCollection.new
named_routes.url_helpers_module.prepend(method_missing_module)
named_routes.path_helpers_module.prepend(method_missing_module)
end

def generate_extras(options, recall = {})
Rails.application&.reload_routes_unless_loaded

super(options, recall)
end

private
def method_missing_module
@method_missing_module ||= Module.new do
private
def method_missing(method_name, *args, &block)
if Rails.application&.reload_routes_unless_loaded
public_send(method_name, *args, &block)
else
super(method_name, *args, &block)
end
end

def respond_to_missing?(method_name, *args)
if Rails.application&.reload_routes_unless_loaded
respond_to?(method_name, *args)
else
super(method_name, *args)
end
end
end
end
end
end
end
3 changes: 2 additions & 1 deletion railties/lib/rails/rack.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

module Rails
module Rack
autoload :Logger, "rails/rack/logger"
autoload :Logger, "rails/rack/logger"
autoload :LoadRoutes, "rails/rack/load_routes"
end
end
20 changes: 20 additions & 0 deletions railties/lib/rails/rack/load_routes.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
# frozen_string_literal: true

module Rails
module Rack
class LoadRoutes
def initialize(app)
@app = app
@called = false
end

def call(env)
@called ||= begin
Rails.application.reload_routes_unless_loaded
true
end
@app.call(env)
end
end
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
require "isolation/abstract_unit"
require "rack/test"

class SharedSetup < ActionController::TestCase
class ActionControllerTestCaseIntegrationTest < ActionController::TestCase
class_attribute :executor_around_each_request

include ActiveSupport::Testing::Isolation
Expand Down Expand Up @@ -54,57 +54,59 @@ def set_current_customer
<%= Current.customer&.name || 'noone' %>,<%= Time.zone.name %>
RUBY

app_file "config/routes.rb", <<~RUBY
Rails.application.routes.draw do
get "/customers/:action", controller: :customers
end
RUBY

require "#{app_path}/config/environment"

@controller = CustomersController.new
@routes = ActionDispatch::Routing::RouteSet.new.tap do |r|
r.draw do
get "/customers/:action", controller: :customers
end
end
@routes = Rails.application.routes
end

teardown :teardown_app
end

class ActionControllerTestCaseWithExecutorIntegrationTest < SharedSetup
self.executor_around_each_request = true
class WithExecutorIntegrationTest < ActionControllerTestCaseIntegrationTest
self.executor_around_each_request = true

test "current customer is cleared after each request" do
assert Rails.application.config.active_support.executor_around_test_case
assert ActionController::TestCase.executor_around_each_request
test "current customer is cleared after each request" do
assert Rails.application.config.active_support.executor_around_test_case
assert ActionController::TestCase.executor_around_each_request

get :get_current_customer
assert_response :ok
assert_match(/noone,UTC/, response.body)
get :get_current_customer
assert_response :ok
assert_match(/noone,UTC/, response.body)

get :set_current_customer
assert_response :ok
assert_match(/david,Copenhagen/, response.body)
get :set_current_customer
assert_response :ok
assert_match(/david,Copenhagen/, response.body)

get :get_current_customer
assert_response :ok
assert_match(/noone,UTC/, response.body)
get :get_current_customer
assert_response :ok
assert_match(/noone,UTC/, response.body)
end
end
end

class ActionControllerTestCaseWithoutExecutorIntegrationTest < SharedSetup
self.executor_around_each_request = false
class WithoutExecutorIntegrationTest < ActionControllerTestCaseIntegrationTest
self.executor_around_each_request = false

test "current customer is not cleared after each request" do
assert_not Rails.application.config.active_support.executor_around_test_case
assert_not ActionController::TestCase.executor_around_each_request
test "current customer is not cleared after each request" do
assert_not Rails.application.config.active_support.executor_around_test_case
assert_not ActionController::TestCase.executor_around_each_request

get :get_current_customer
assert_response :ok
assert_match(/noone,UTC/, response.body)
get :get_current_customer
assert_response :ok
assert_match(/noone,UTC/, response.body)

get :set_current_customer
assert_response :ok
assert_match(/david,Copenhagen/, response.body)
get :set_current_customer
assert_response :ok
assert_match(/david,Copenhagen/, response.body)

get :get_current_customer
assert_response :ok
assert_match(/david,Copenhagen/, response.body)
get :get_current_customer
assert_response :ok
assert_match(/david,Copenhagen/, response.body)
end
end
end
Loading

0 comments on commit 7ac3338

Please sign in to comment.