Skip to content

Commit b40d92e

Browse files
committed
Fix routes being cleared when using reload_routes!:
- Pretty sure it fixes rails#54297 and rails#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 rails#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. railsware/js-routes#320 and amatsuda/traceroute#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.
1 parent b1d1945 commit b40d92e

File tree

4 files changed

+35
-2
lines changed

4 files changed

+35
-2
lines changed

actionpack/CHANGELOG.md

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,12 @@
1+
* Fix `Rails.application.reload_routes!` from clearing almost all routes.
2+
3+
When calling `Rails.application.reload_routes!` inside a middleware of
4+
a Rake task, it was possible under certain conditions that all routes would be cleared.
5+
If ran inside a middleware, this would result in getting a 404 on most page you visit.
6+
This issue was only happening in development.
7+
8+
*Edouard Chin*
9+
110
* Add resource name to the `ArgumentError` that's raised when invalid `:only` or `:except` options are given to `#resource` or `#resources`
211

312
This makes it easier to locate the source of the problem, especially for routes drawn by gems.

railties/lib/rails/application.rb

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -156,7 +156,11 @@ def run_load_hooks! # :nodoc:
156156

157157
# Reload application routes regardless if they changed or not.
158158
def reload_routes!
159-
routes_reloader.reload!
159+
if routes_reloader.execute_unless_loaded
160+
routes_reloader.loaded = false
161+
else
162+
routes_reloader.reload!
163+
end
160164
end
161165

162166
def reload_routes_unless_loaded # :nodoc:

railties/lib/rails/application/routes_reloader.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ class RoutesReloader
88

99
attr_reader :route_sets, :paths, :external_routes, :loaded
1010
attr_accessor :eager_load
11-
attr_writer :run_after_load_paths # :nodoc:
11+
attr_writer :run_after_load_paths, :loaded # :nodoc:
1212
delegate :execute_if_updated, :updated?, to: :updater
1313

1414
def initialize

railties/test/application/rake_test.rb

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -185,6 +185,26 @@ def test_code_statistics
185185
assert_match(/Code LOC: \d+\s+Test LOC: \d+\s+ Code to Test Ratio: 1:\w+/, rails("stats"))
186186
end
187187

188+
def test_reload_routes
189+
add_to_config <<-RUBY
190+
rake_tasks do
191+
task do_something: :environment do
192+
Rails.application.reload_routes!
193+
puts Rails.application.routes.named_routes.to_h.keys.join(" ")
194+
end
195+
end
196+
RUBY
197+
198+
app_file "config/routes.rb", <<-RUBY
199+
Rails.application.routes.draw do
200+
get "foo", to: "foo#bar", as: :my_great_route
201+
end
202+
RUBY
203+
204+
output = Dir.chdir(app_path) { `bin/rails do_something` }
205+
assert_includes(output.lines.first, "my_great_route")
206+
end
207+
188208
def test_loading_specific_fixtures
189209
rails "generate", "model", "user", "username:string", "password:string"
190210
rails "generate", "model", "product", "name:string"

0 commit comments

Comments
 (0)