diff --git a/actionpack/lib/action_dispatch/testing/assertions/routing.rb b/actionpack/lib/action_dispatch/testing/assertions/routing.rb index bef88b1bdf7..7d8594e1c59 100644 --- a/actionpack/lib/action_dispatch/testing/assertions/routing.rb +++ b/actionpack/lib/action_dispatch/testing/assertions/routing.rb @@ -46,7 +46,9 @@ module ActionDispatch 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 diff --git a/actionpack/test/abstract_unit.rb b/actionpack/test/abstract_unit.rb index 7d66ec07355..7b536af0c84 100644 --- a/actionpack/test/abstract_unit.rb +++ b/actionpack/test/abstract_unit.rb @@ -34,6 +34,8 @@ module Rails @_env ||= ActiveSupport::StringInquirer.new(ENV["RAILS_ENV"] || ENV["RACK_ENV"] || "test") end + def application; end + def root; end end end diff --git a/actionpack/test/dispatch/routing_assertions_test.rb b/actionpack/test/dispatch/routing_assertions_test.rb index 862dfc646b2..335bfd95348 100644 --- a/actionpack/test/dispatch/routing_assertions_test.rb +++ b/actionpack/test/dispatch/routing_assertions_test.rb @@ -4,7 +4,11 @@ require "abstract_unit" 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 @@ -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 @@ -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 diff --git a/railties/CHANGELOG.md b/railties/CHANGELOG.md index ee6c9d868aa..ed1c9966635 100644 --- a/railties/CHANGELOG.md +++ b/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 Rubocop and GitHub Actions to plugin generator. This can be skipped using --skip-rubocop and --skip-ci. diff --git a/railties/lib/rails/application.rb b/railties/lib/rails/application.rb index bf20f1f29bf..1355d2090a6 100644 --- a/railties/lib/rails/application.rb +++ b/railties/lib/rails/application.rb @@ -9,6 +9,7 @@ require "active_support/deprecation" 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" @@ -153,6 +154,10 @@ module Rails 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 diff --git a/railties/lib/rails/application/default_middleware_stack.rb b/railties/lib/rails/application/default_middleware_stack.rb index 738e41f9427..d16f0aad9a9 100644 --- a/railties/lib/rails/application/default_middleware_stack.rb +++ b/railties/lib/rails/application/default_middleware_stack.rb @@ -62,6 +62,8 @@ module Rails 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 diff --git a/railties/lib/rails/application/finisher.rb b/railties/lib/rails/application/finisher.rb index 6a0f9268c0f..61a7be9291b 100644 --- a/railties/lib/rails/application/finisher.rb +++ b/railties/lib/rails/application/finisher.rb @@ -159,7 +159,6 @@ module Rails 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 @@ -177,7 +176,12 @@ module Rails 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 diff --git a/railties/lib/rails/application/routes_reloader.rb b/railties/lib/rails/application/routes_reloader.rb index 569766dc5f6..8c1d1b34a13 100644 --- a/railties/lib/rails/application/routes_reloader.rb +++ b/railties/lib/rails/application/routes_reloader.rb @@ -7,7 +7,7 @@ module Rails 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 @@ -17,9 +17,11 @@ module Rails @route_sets = [] @external_routes = [] @eager_load = false + @loaded = false end def reload! + @loaded = true clear! load_paths finalize! @@ -28,6 +30,14 @@ module Rails 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 diff --git a/railties/lib/rails/commands/routes/routes_command.rb b/railties/lib/rails/commands/routes/routes_command.rb index c979a9014b8..2bef9fa2e09 100644 --- a/railties/lib/rails/commands/routes/routes_command.rb +++ b/railties/lib/rails/commands/routes/routes_command.rb @@ -23,6 +23,7 @@ module Rails 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) diff --git a/railties/lib/rails/commands/unused_routes/unused_routes_command.rb b/railties/lib/rails/commands/unused_routes/unused_routes_command.rb index 9fd4fbff148..d4c55125e47 100644 --- a/railties/lib/rails/commands/unused_routes/unused_routes_command.rb +++ b/railties/lib/rails/commands/unused_routes/unused_routes_command.rb @@ -41,6 +41,7 @@ module Rails def perform(*) boot_application! + Rails.application.reload_routes_unless_loaded require "action_dispatch/routing/inspector" say(inspector.format(formatter, routes_filter)) diff --git a/railties/lib/rails/engine.rb b/railties/lib/rails/engine.rb index 5d010ae5830..3d17061646f 100644 --- a/railties/lib/rails/engine.rb +++ b/railties/lib/rails/engine.rb @@ -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 @@ -543,7 +544,7 @@ module Rails # 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 diff --git a/railties/lib/rails/engine/route_set.rb b/railties/lib/rails/engine/route_set.rb new file mode 100644 index 00000000000..0166ec6d26d --- /dev/null +++ b/railties/lib/rails/engine/route_set.rb @@ -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 diff --git a/railties/lib/rails/rack.rb b/railties/lib/rails/rack.rb index 579fb25cc4b..07b90069131 100644 --- a/railties/lib/rails/rack.rb +++ b/railties/lib/rails/rack.rb @@ -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 diff --git a/railties/lib/rails/rack/load_routes.rb b/railties/lib/rails/rack/load_routes.rb new file mode 100644 index 00000000000..6d88b54786e --- /dev/null +++ b/railties/lib/rails/rack/load_routes.rb @@ -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 diff --git a/railties/test/application/action_controller_test_case_integration_test.rb b/railties/test/application/action_controller_test_case_integration_test.rb index eb7acfbda12..4a7b84956bc 100644 --- a/railties/test/application/action_controller_test_case_integration_test.rb +++ b/railties/test/application/action_controller_test_case_integration_test.rb @@ -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 @@ -54,57 +54,59 @@ class SharedSetup < ActionController::TestCase <%= 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) - end -end - -class ActionControllerTestCaseWithoutExecutorIntegrationTest < SharedSetup - 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 - - 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 :get_current_customer - assert_response :ok - assert_match(/david,Copenhagen/, response.body) + get :get_current_customer + assert_response :ok + assert_match(/noone,UTC/, response.body) + end + end + + 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 + + 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 :get_current_customer + assert_response :ok + assert_match(/david,Copenhagen/, response.body) + end end end diff --git a/railties/test/application/action_view_test_integration_case_test.rb b/railties/test/application/action_view_test_integration_case_test.rb new file mode 100644 index 00000000000..e7d53681958 --- /dev/null +++ b/railties/test/application/action_view_test_integration_case_test.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +require "isolation/abstract_unit" + +class ActionViewTestCaseIntegrationTest < ActionView::TestCase + include ActiveSupport::Testing::Isolation + + class HomeController < ActionController::Base + def index; end + + def url_options + {} + end + end + + setup do + build_app + + app_file "config/routes.rb", <<~RUBY + Rails.application.routes.draw do + root to: "action_view_test_case_test/home#index" + end + RUBY + + require "#{app_path}/config/environment" + + HomeController.include(Rails.application.routes.url_helpers) + @controller = HomeController.new + end + + teardown :teardown_app + + test "can use url helpers" do + assert_equal("/", root_path) + end +end diff --git a/railties/test/application/initializers/frameworks_test.rb b/railties/test/application/initializers/frameworks_test.rb index 28f9d9b1903..872352a0376 100644 --- a/railties/test/application/initializers/frameworks_test.rb +++ b/railties/test/application/initializers/frameworks_test.rb @@ -67,8 +67,8 @@ module ApplicationTests 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 diff --git a/railties/test/application/loading_test.rb b/railties/test/application/loading_test.rb index 360b550698c..e655e3380cc 100644 --- a/railties/test/application/loading_test.rb +++ b/railties/test/application/loading_test.rb @@ -310,6 +310,7 @@ class LoadingTest < ActiveSupport::TestCase Rails.configuration.after_routes_loaded do $counter *= 3 end + Rails.application.reload_routes! RUBY app_file "app/models/user.rb", <<-MODEL @@ -373,6 +374,7 @@ class LoadingTest < ActiveSupport::TestCase Rails.configuration.after_routes_loaded do $counter *= 3 end + Rails.application.reload_routes! RUBY boot_app "development" diff --git a/railties/test/application/rack/load_routes_test.rb b/railties/test/application/rack/load_routes_test.rb new file mode 100644 index 00000000000..d25aeac5fbf --- /dev/null +++ b/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, :reload_routes_unless_loaded, 1) do + 5.times { get "/test" } + end + end + end + end +end diff --git a/railties/test/application/route_set_test.rb b/railties/test/application/route_set_test.rb new file mode 100644 index 00000000000..531150aa3d1 --- /dev/null +++ b/railties/test/application/route_set_test.rb @@ -0,0 +1,102 @@ +# frozen_string_literal: true + +require "isolation/abstract_unit" + +module Rails + class Engine + class RouteSetTest < ActiveSupport::TestCase + include ActiveSupport::Testing::Isolation + + setup :build_app + + teardown :teardown_app + + test "app lazily loads routes when invoking url helpers" do + require "#{app_path}/config/environment" + + assert_not_operator(:root_path, :in?, app_url_helpers.methods) + assert_equal("/", app_url_helpers.root_path) + end + + test "engine lazily loads routes when invoking url helpers" do + require "#{app_path}/config/environment" + + assert_not_operator(:root_path, :in?, engine_url_helpers.methods) + assert_equal("/plugin/", engine_url_helpers.root_path) + end + + test "app lazily loads routes when checking respond_to?" do + require "#{app_path}/config/environment" + + assert_not_operator(:root_path, :in?, app_url_helpers.methods) + assert_operator(app_url_helpers, :respond_to?, :root_path) + end + + test "engine lazily loads routes when checking respond_to?" do + require "#{app_path}/config/environment" + + assert_not_operator(:root_path, :in?, engine_url_helpers.methods) + assert_operator(engine_url_helpers, :respond_to?, :root_path) + end + + test "app lazily loads routes when making a request" do + require "#{app_path}/config/environment" + + @app = Rails.application + + assert_not_operator(:root_path, :in?, app_url_helpers.methods) + response = get("/") + assert_equal(200, response.first) + end + + test "engine lazily loads routes when making a request" do + require "#{app_path}/config/environment" + + @app = Rails.application + + assert_not_operator(:root_path, :in?, engine_url_helpers.methods) + response = get("/plugin/") + assert_equal(200, response.first) + end + + private + def build_app + super + + app_file "config/routes.rb", <<-RUBY + Rails.application.routes.draw do + root to: proc { [200, {}, []] } + + mount Plugin::Engine, at: "/plugin" + end + RUBY + + build_engine + end + + def build_engine + engine "plugin" do |plugin| + plugin.write "lib/plugin.rb", <<~RUBY + module Plugin + class Engine < ::Rails::Engine + end + end + RUBY + plugin.write "config/routes.rb", <<~RUBY + Plugin::Engine.routes.draw do + root to: proc { [200, {}, []] } + end + RUBY + end + end + + def app_url_helpers + Rails.application.routes.url_helpers + end + + def engine_url_helpers + Plugin::Engine.routes.url_helpers + end + end + end +end diff --git a/railties/test/application/url_generation_test.rb b/railties/test/application/url_generation_test.rb index dc737089f63..35e995e37a1 100644 --- a/railties/test/application/url_generation_test.rb +++ b/railties/test/application/url_generation_test.rb @@ -21,6 +21,7 @@ module ApplicationTests config.eager_load = false config.hosts << proc { true } config.secret_key_base = "b3c631c314c0bbca50c1b2843150fe33" + config.middleware.delete Rails::Rack::LoadRoutes end Rails.application.initialize! diff --git a/railties/test/commands/middleware_test.rb b/railties/test/commands/middleware_test.rb index 70b6f875826..b0373610ec8 100644 --- a/railties/test/commands/middleware_test.rb +++ b/railties/test/commands/middleware_test.rb @@ -39,6 +39,7 @@ class Rails::Command::MiddlewareTest < ActiveSupport::TestCase "Rails::Rack::Logger", "ActionDispatch::ShowExceptions", "ActionDispatch::DebugExceptions", + "Rails::Rack::LoadRoutes", "ActionDispatch::Reloader", "ActionDispatch::Callbacks", "ActiveRecord::Migration::CheckPending", @@ -76,6 +77,7 @@ class Rails::Command::MiddlewareTest < ActiveSupport::TestCase "ActionDispatch::ShowExceptions", "ActionDispatch::DebugExceptions", "ActionDispatch::ActionableExceptions", + "Rails::Rack::LoadRoutes", "ActionDispatch::Reloader", "ActionDispatch::Callbacks", "ActiveRecord::Migration::CheckPending", @@ -108,6 +110,7 @@ class Rails::Command::MiddlewareTest < ActiveSupport::TestCase "Rails::Rack::Logger", "ActionDispatch::ShowExceptions", "ActionDispatch::DebugExceptions", + "Rails::Rack::LoadRoutes", "ActionDispatch::Reloader", "ActionDispatch::Callbacks", "Rack::Head", diff --git a/railties/test/isolation/abstract_unit.rb b/railties/test/isolation/abstract_unit.rb index 8f2edb8ba87..a04820143fd 100644 --- a/railties/test/isolation/abstract_unit.rb +++ b/railties/test/isolation/abstract_unit.rb @@ -265,6 +265,7 @@ module TestHelpers @app.config.active_support.deprecation = :log @app.config.log_level = :error @app.config.secret_key_base = "b3c631c314c0bbca50c1b2843150fe33" + @app.config.middleware.delete Rails::Rack::LoadRoutes yield @app if block_given? @app.initialize! diff --git a/railties/test/railties/engine_test.rb b/railties/test/railties/engine_test.rb index 81782a1842c..9004b4fa8d7 100644 --- a/railties/test/railties/engine_test.rb +++ b/railties/test/railties/engine_test.rb @@ -950,8 +950,8 @@ en: assert_equal "bukkits_", Bukkits.table_name_prefix assert_equal "bukkits", Bukkits::Engine.engine_name assert_equal Bukkits.railtie_namespace, Bukkits::Engine - assert ::Bukkits::MyMailer.method_defined?(:foo_url) - assert_not ::Bukkits::MyMailer.method_defined?(:bar_url) + assert ::Bukkits::MyMailer.new.respond_to?(:foo_url) + assert_not ::Bukkits::MyMailer.new.respond_to?(:bar_url) get("/bar") assert_equal "/bar", last_response.body