From 0ad9f8c28b9b13cb2947de46dadf76c5008efed4 Mon Sep 17 00:00:00 2001 From: Edouard CHIN Date: Tue, 21 Jan 2025 05:20:19 +0100 Subject: [PATCH] Fix routes being cleared when using `reload_routes!`: - Pretty sure it fixes #54297 and #53205, but both issues didn't provide enough information. - When using `Rails.application.reload_routes!` (which is public API), in certain conditions, it is possible that this will clear almost all routes in the routeset. The line of code making this happen is this one https://github.com/rails/rails/blob/38e9a72db45842d1d2f05fb2272df3a10225f981/actionpack/lib/action_dispatch/routing/route_set.rb#L460. That flag is supposed to be `true` for the entire duration of evaluating a route file. This was the case before https://github.com/rails/rails/pull/53522, which introduced the issue. It indirectly resets that flag to false because of a recursion. This is a summary of the code order execution when calling `Rails.application.reload_routes`: 1) The RouteSet sets the `disable_clear_and_finalize` flag to false, so that no routes gets cleared in between multiple call to `Rails.application.routes.draw`. 2) All route files get evaluated, and any call to `Rails.application.routes.draw` gets picked up. 3) We enter a recursion because the RouteSet in development is a LazyRouteSet which when calling `draw` reloads the route. [here](https://github.com/rails/rails/blob/38e9a72db45842d1d2f05fb2272df3a10225f981/railties/lib/rails/engine/lazy_route_set.rb#L72) and [here](https://github.com/rails/rails/blob/38e9a72db45842d1d2f05fb2272df3a10225f981/railties/lib/rails/application.rb#L163) 4) The route file get evaluated again, the `disable_clear_and_finalize` is reset to false, we exit the recursion and continue where we were. 5) The `disable_clear_and_finalize` is now false, any call to `Rails.application.draw` now resets all routes before it gets evaluated. Which basically means the very last one would have effect. ------------------------ I can only think of two way where `reload_routes` may cause a issue: - Inside a Rake task - Inside a middleware This is because the application needs to be initialized and the routeset never evaluated in order to trigger the bug. https://github.com/railsware/js-routes/pull/320 and https://github.com/amatsuda/traceroute/issues/49 confirms it. ## Solution When calling `reload_routes!`, if the routes were never loaded, load them and set the `loaded` flag. Otherwise, also load them but with the difference that with the `loaded` flag set, we won't reset all routes. --- actionpack/CHANGELOG.md | 9 +++++++++ railties/lib/rails/application.rb | 2 +- railties/test/application/rake_test.rb | 20 ++++++++++++++++++++ 3 files changed, 30 insertions(+), 1 deletion(-) diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md index bdccfb9805170..ab38408868e9f 100644 --- a/actionpack/CHANGELOG.md +++ b/actionpack/CHANGELOG.md @@ -1,3 +1,12 @@ +* Fix `Rails.application.reload_routes!` from clearing almost all routes. + + When calling `Rails.application.reload_routes!` inside a middleware of + a Rake task, it was possible under certain conditions that all routes would be cleared. + If ran inside a middleware, this would result in getting a 404 on most page you visit. + This issue was only happening in development. + + *Edouard Chin* + * Add resource name to the `ArgumentError` that's raised when invalid `:only` or `:except` options are given to `#resource` or `#resources` This makes it easier to locate the source of the problem, especially for routes drawn by gems. diff --git a/railties/lib/rails/application.rb b/railties/lib/rails/application.rb index 1396613d52310..b79b41e844459 100644 --- a/railties/lib/rails/application.rb +++ b/railties/lib/rails/application.rb @@ -156,7 +156,7 @@ def run_load_hooks! # :nodoc: # Reload application routes regardless if they changed or not. def reload_routes! - routes_reloader.reload! + routes_reloader.execute_unless_loaded || routes_reloader.reload! end def reload_routes_unless_loaded # :nodoc: diff --git a/railties/test/application/rake_test.rb b/railties/test/application/rake_test.rb index 75e33bd274ce6..cecc04955473f 100644 --- a/railties/test/application/rake_test.rb +++ b/railties/test/application/rake_test.rb @@ -185,6 +185,26 @@ def test_code_statistics assert_match(/Code LOC: \d+\s+Test LOC: \d+\s+ Code to Test Ratio: 1:\w+/, rails("stats")) end + def test_reload_routes + add_to_config <<-RUBY + rake_tasks do + task do_something: :environment do + Rails.application.reload_routes! + puts Rails.application.routes.named_routes.to_h.keys.join(" ") + end + end + RUBY + + app_file "config/routes.rb", <<-RUBY + Rails.application.routes.draw do + get "foo", to: "foo#bar", as: :my_great_route + end + RUBY + + output = Dir.chdir(app_path) { `bin/rails do_something` } + assert_includes(output.lines.first, "my_great_route") + end + def test_loading_specific_fixtures rails "generate", "model", "user", "username:string", "password:string" rails "generate", "model", "product", "name:string"