Merge pull request #52012 from Shopify/defer_route_drawing

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

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

@@ -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
 

actionpack/test/abstract_unit.rb

@@ -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

actionpack/test/dispatch/routing_assertions_test.rb

@@ -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
 
@@ -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

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.
 

railties/lib/rails/application.rb

@@ -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"
 
@@ -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

railties/lib/rails/application/default_middleware_stack.rb

@@ -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

railties/lib/rails/application/finisher.rb

@@ -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
@@ -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

railties/lib/rails/application/routes_reloader.rb

@@ -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
@@ -17,9 +17,11 @@ def initialize
         @route_sets = []
         @external_routes = []
         @eager_load = false
+        @loaded = false
       end
 
       def reload!
+        @loaded = true
         clear!
         load_paths
         finalize!
@@ -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

railties/lib/rails/commands/routes/routes_command.rb

@@ -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)

railties/lib/rails/commands/unused_routes/unused_routes_command.rb

@@ -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))

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 @@ 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

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

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

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

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 @@ 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|
+    @routes = Rails.application.routes
-      r.draw do
-        get "/customers/:action", controller: :customers
-      end
-    end
   end
 
   teardown :teardown_app
-end
 
-class ActionControllerTestCaseWithExecutorIntegrationTest < SharedSetup
+  class WithExecutorIntegrationTest < ActionControllerTestCaseIntegrationTest
-  self.executor_around_each_request = true
+    self.executor_around_each_request = true
 
-  test "current customer is cleared after each request" do
+    test "current customer is cleared after each request" do
-    assert Rails.application.config.active_support.executor_around_test_case
+      assert Rails.application.config.active_support.executor_around_test_case
-    assert ActionController::TestCase.executor_around_each_request
+      assert ActionController::TestCase.executor_around_each_request
 
-    get :get_current_customer
+      get :get_current_customer
-    assert_response :ok
+      assert_response :ok
-    assert_match(/noone,UTC/, response.body)
+      assert_match(/noone,UTC/, response.body)
 
-    get :set_current_customer
+      get :set_current_customer
-    assert_response :ok
+      assert_response :ok
-    assert_match(/david,Copenhagen/, response.body)
+      assert_match(/david,Copenhagen/, response.body)
 
-    get :get_current_customer
+      get :get_current_customer
-    assert_response :ok
+      assert_response :ok
-    assert_match(/noone,UTC/, response.body)
+      assert_match(/noone,UTC/, response.body)
-  end
+    end
-end
+  end
-
+
-class ActionControllerTestCaseWithoutExecutorIntegrationTest < SharedSetup
+  class WithoutExecutorIntegrationTest < ActionControllerTestCaseIntegrationTest
-  self.executor_around_each_request = false
+    self.executor_around_each_request = false
-
+
-  test "current customer is not cleared after each request" do
+    test "current customer is not cleared after each request" do
-    assert_not Rails.application.config.active_support.executor_around_test_case
+      assert_not Rails.application.config.active_support.executor_around_test_case
-    assert_not ActionController::TestCase.executor_around_each_request
+      assert_not ActionController::TestCase.executor_around_each_request
-
+
-    get :get_current_customer
+      get :get_current_customer
-    assert_response :ok
+      assert_response :ok
-    assert_match(/noone,UTC/, response.body)
+      assert_match(/noone,UTC/, response.body)
-
+
-    get :set_current_customer
+      get :set_current_customer
-    assert_response :ok
+      assert_response :ok
-    assert_match(/david,Copenhagen/, response.body)
+      assert_match(/david,Copenhagen/, response.body)
-
+
-    get :get_current_customer
+      get :get_current_customer
-    assert_response :ok
+      assert_response :ok
-    assert_match(/david,Copenhagen/, response.body)
+      assert_match(/david,Copenhagen/, response.body)
+    end
   end
 end

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

railties/test/application/initializers/frameworks_test.rb

@@ -67,8 +67,8 @@ def notify
       RUBY
 
       app("development")
-      assert Foo.method_defined?(:foo_url)
+      assert Foo.new.respond_to?(:foo_url)
-      assert Foo.method_defined?(:main_app)
+      assert Foo.new.respond_to?(:main_app)
     end
 
     test "allows to not load all helpers for controllers" do

railties/test/application/loading_test.rb

@@ -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
@@ -373,6 +374,7 @@ class User
       Rails.configuration.after_routes_loaded do
         $counter *= 3
       end
+      Rails.application.reload_routes!
     RUBY
 
     boot_app "development"

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

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

railties/test/application/url_generation_test.rb

@@ -21,6 +21,7 @@ class MyApp < Rails::Application
         config.eager_load = false
         config.hosts << proc { true }
         config.secret_key_base = "b3c631c314c0bbca50c1b2843150fe33"
+        config.middleware.delete Rails::Rack::LoadRoutes
       end
 
       Rails.application.initialize!

railties/test/commands/middleware_test.rb

@@ -39,6 +39,7 @@ def app
       "Rails::Rack::Logger",
       "ActionDispatch::ShowExceptions",
       "ActionDispatch::DebugExceptions",
+      "Rails::Rack::LoadRoutes",
       "ActionDispatch::Reloader",
       "ActionDispatch::Callbacks",
       "ActiveRecord::Migration::CheckPending",
@@ -76,6 +77,7 @@ def app
       "ActionDispatch::ShowExceptions",
       "ActionDispatch::DebugExceptions",
       "ActionDispatch::ActionableExceptions",
+      "Rails::Rack::LoadRoutes",
       "ActionDispatch::Reloader",
       "ActionDispatch::Callbacks",
       "ActiveRecord::Migration::CheckPending",
@@ -108,6 +110,7 @@ def app
       "Rails::Rack::Logger",
       "ActionDispatch::ShowExceptions",
       "ActionDispatch::DebugExceptions",
+      "Rails::Rack::LoadRoutes",
       "ActionDispatch::Reloader",
       "ActionDispatch::Callbacks",
       "Rack::Head",

railties/test/isolation/abstract_unit.rb

@@ -265,6 +265,7 @@ def self.name; "RailtiesTestApp"; end
       @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!

railties/test/railties/engine_test.rb

@@ -950,8 +950,8 @@ class MyMailer < ActionMailer::Base
       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 ::Bukkits::MyMailer.new.respond_to?(:foo_url)
-      assert_not ::Bukkits::MyMailer.method_defined?(:bar_url)
+      assert_not ::Bukkits::MyMailer.new.respond_to?(:bar_url)
 
       get("/bar")
       assert_equal "/bar", last_response.body