Skip to content

Commit

Permalink
Fix routes being cleared when using reload_routes!:
Browse files Browse the repository at this point in the history
- 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.
  • Loading branch information
Edouard-chin committed Jan 27, 2025
1 parent fd4e570 commit 0ad9f8c
Show file tree
Hide file tree
Showing 3 changed files with 30 additions and 1 deletion.
9 changes: 9 additions & 0 deletions actionpack/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -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.
Expand Down
2 changes: 1 addition & 1 deletion railties/lib/rails/application.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
20 changes: 20 additions & 0 deletions railties/test/application/rake_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down

0 comments on commit 0ad9f8c

Please sign in to comment.