Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Defer route drawing to the first request, or when url_helpers called #51614

Merged
merged 1 commit into from May 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 3 additions & 1 deletion actionpack/lib/action_dispatch/testing/assertions/routing.rb
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
32 changes: 31 additions & 1 deletion actionpack/test/dispatch/routing_assertions_test.rb
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
@@ -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 options to bin/rails app:update.

`bin/rails app:update` now supports the same generic options that generators do:
Expand Down
1 change: 1 addition & 0 deletions railties/lib/rails/application.rb
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
2 changes: 2 additions & 0 deletions railties/lib/rails/application/default_middleware_stack.rb
Expand Up @@ -62,6 +62,8 @@ def build_stack
middleware.use ::ActionDispatch::ActionableExceptions
end

middleware.use ::Rails::Rack::LoadRoutes, app.routes_reloader 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
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
11 changes: 10 additions & 1 deletion railties/lib/rails/application/routes_reloader.rb
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,13 @@ def reload!
revert
end

def execute_unless_loaded
unless @loaded
execute
true
end
end

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

say inspector.format(formatter, routes_filter)
Expand Down
Expand Up @@ -41,6 +41,7 @@ def action_missing?

def perform(*)
boot_application!
Rails.application.routes_reloader.execute_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
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
43 changes: 43 additions & 0 deletions railties/lib/rails/engine/route_set.rb
@@ -0,0 +1,43 @@
# frozen_string_literal: true

# :markup: markdown

require "action_dispatch/routing/route_set"

module Rails
class Engine
class RouteSet < ActionDispatch::Routing::RouteSet # :nodoc:
def initialize(config = DEFAULT_CONFIG)
super
named_routes.url_helpers_module.prepend(method_missing_module)
named_routes.path_helpers_module.prepend(method_missing_module)
end

private
def method_missing_module
@method_missing_module ||= Module.new do
private
def method_missing(method_name, *args, &block)
application = Rails.application
if application && application.initialized? && application.routes_reloader.execute_unless_loaded
ActiveSupport.run_load_hooks(:after_routes_loaded, application)
public_send(method_name, *args, &block)
else
super(method_name, *args, &block)
end
end

def respond_to_missing?(method_name, *args)
application = Rails.application
if application && application.initialized? && application.routes_reloader.execute_unless_loaded
ActiveSupport.run_load_hooks(:after_routes_loaded, application)
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
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
21 changes: 21 additions & 0 deletions railties/lib/rails/rack/load_routes.rb
@@ -0,0 +1,21 @@
# frozen_string_literal: true

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

def call(env)
@called ||= begin
@routes_reloader.execute_unless_loaded
true
end
@app.call(env)
end
end
end
end
4 changes: 2 additions & 2 deletions railties/test/application/initializers/frameworks_test.rb
Expand Up @@ -67,8 +67,8 @@ def notify
RUBY

app("development")
assert Foo.method_defined?(:foo_url)
assert Foo.method_defined?(:main_app)
assert Foo.new.respond_to?(:foo_url)
assert Foo.new.respond_to?(:main_app)
end

test "allows to not load all helpers for controllers" do
Expand Down
2 changes: 2 additions & 0 deletions railties/test/application/loading_test.rb
Expand Up @@ -310,6 +310,7 @@ class User
Rails.configuration.after_routes_loaded do
$counter *= 3
end
Rails.application.reload_routes!
RUBY

app_file "app/models/user.rb", <<-MODEL
Expand Down Expand Up @@ -373,6 +374,7 @@ class User
Rails.configuration.after_routes_loaded do
$counter *= 3
end
Rails.application.reload_routes!
RUBY

boot_app "development"
Expand Down
38 changes: 38 additions & 0 deletions railties/test/application/rack/load_routes_test.rb
@@ -0,0 +1,38 @@
# frozen_string_literal: true

require "isolation/abstract_unit"
require "active_support/log_subscriber/test_helper"
require "rack/test"

module ApplicationTests
module RackTests
class LoggerTest < ActiveSupport::TestCase
include ActiveSupport::Testing::Isolation
include ActiveSupport::LogSubscriber::TestHelper
include Rack::Test::Methods

setup do
build_app
require "#{app_path}/config/environment"
end

teardown do
teardown_app
end

test "loads routes on request" do
assert_equal(false, Rails.application.routes_reloader.loaded)

get "/test"

assert_equal(true, Rails.application.routes_reloader.loaded)
end

test "loads routes only once" do
assert_called(Rails.application.routes_reloader, :execute_unless_loaded, 1) do
5.times { get "/test" }
end
end
end
end
end