Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add routes --unused option to detect extraneous routes. #45701

Merged
merged 1 commit into from
Aug 5, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions actionpack/lib/action_dispatch/journey/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ class Routes # :nodoc:

attr_reader :routes, :custom_routes, :anchored_routes

def initialize
@routes = []
def initialize(routes = [])
@routes = routes
@ast = nil
@anchored_routes = []
@custom_routes = []
Expand Down
21 changes: 21 additions & 0 deletions actionpack/lib/action_dispatch/routing/inspector.rb
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,27 @@ def route_header(index:)
"--[ Route #{index} ]".ljust(@width, "-")
end
end

class Unused < Sheet
def header(routes)
@buffer << <<~MSG
Found #{routes.count} unused #{"route".pluralize(routes.count)}:
MSG

super
end

def no_routes(routes, filter)
@buffer <<
if filter.none?
"No unused routes found."
elsif filter.key?(:controller)
"No unused routes found for this controller."
elsif filter.key?(:grep)
"No unused routes found for this grep pattern."
end
end
end
end

class HtmlTableFormatter
Expand Down
16 changes: 16 additions & 0 deletions railties/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,19 @@
* Add `routes --unused` option to detect extraneous routes.

Example:

```
> bin/rails rails --unused

Found 2 unused routes:

Prefix Verb URI Pattern Controller#Action
one GET /one(.:format) action#one
two GET /two(.:format) action#two
Comment on lines +10 to +12

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't this?

    Prefix Verb URI Pattern    Controller#Action
       one GET  /one(.:format) controller#one
       two GET  /two(.:format) controller#two

```

*Gannon McGibbon*

* Add `--parent` option to controller generator to specify parent class of job.

Example:
Expand Down
9 changes: 9 additions & 0 deletions railties/lib/rails/commands/routes/routes_command.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,15 @@ class RoutesCommand < Base # :nodoc:
class_option :controller, aliases: "-c", desc: "Filter by a specific controller, e.g. PostsController or Admin::PostsController."
class_option :grep, aliases: "-g", desc: "Grep routes by a specific pattern."
class_option :expanded, type: :boolean, aliases: "-E", desc: "Print routes expanded vertically with parts explained."
class_option :unused, type: :boolean, aliases: "-u", desc: "Print unused routes."

def invoke_command(*)
if options.key?("unused")
Rails::Command.invoke "unused_routes", ARGV
else
super
end
end

def perform(*)
require_application_and_environment!
Expand Down
75 changes: 75 additions & 0 deletions railties/lib/rails/commands/unused_routes/unused_routes_command.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
# frozen_string_literal: true

require "rails/commands/routes/routes_command"

module Rails
module Command
class UnusedRoutesCommand < Rails::Command::Base # :nodoc:
hide_command!
class_option :controller, aliases: "-c", desc: "Filter by a specific controller, e.g. PostsController or Admin::PostsController."
class_option :grep, aliases: "-g", desc: "Grep routes by a specific pattern."

class RouteInfo
def initialize(route)
requirements = route.requirements
@controller_name = requirements[:controller]
@action_name = requirements[:action]
@controller_class = (@controller_name.to_s.camelize + "Controller").safe_constantize
end

def unused?
controller_class_missing? || (action_missing? && template_missing?)
end

private
def view_path(root)
File.join(root.path, @controller_name, @action_name)
end

def controller_class_missing?
@controller_name && @controller_class.nil?
end

def template_missing?
@controller_class && @controller_class.try(:view_paths).to_a.flat_map { |path| Dir["#{view_path(path)}.*"] }.none?
end

def action_missing?
@controller_class && @controller_class.instance_methods.exclude?(@action_name.to_sym)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this should be checking @controller_class.action_methods instead. And we might want to check if the controller responds to action_missing as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

action_missing I'm a little hesitant about because we don't know what action action_missing is handling, so I wouldn't call a route valid if the controller has that defined. We can definitely try to denote this in the output though!

end
end

def perform(*)
require_application_and_environment!
require "action_dispatch/routing/inspector"

say(inspector.format(formatter, routes_filter))

exit(1) if routes.any?
end

private
def inspector
ActionDispatch::Routing::RoutesInspector.new(routes)
end

def routes
@routes ||= begin
routes = Rails.application.routes.routes.select do |route|
RouteInfo.new(route).unused?
end

ActionDispatch::Journey::Routes.new(routes)
end
end

def formatter
ActionDispatch::Routing::ConsoleFormatter::Unused.new
end

def routes_filter
options.symbolize_keys.slice(:controller, :grep)
end
end
end
end
17 changes: 14 additions & 3 deletions railties/test/commands/routes_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,7 @@ class Rails::Command::RoutesTest < ActiveSupport::TestCase
URI | /rails/conductor/action_mailbox/inbound_emails(.:format)
Controller#Action | rails/conductor/action_mailbox/inbound_emails#index
--[ Route 9 ]--------------
Prefix |
Prefix |#{" "}
Verb | POST
URI | /rails/conductor/action_mailbox/inbound_emails(.:format)
Controller#Action | rails/conductor/action_mailbox/inbound_emails#create
Expand Down Expand Up @@ -296,7 +296,7 @@ class Rails::Command::RoutesTest < ActiveSupport::TestCase
URI | /rails/active_storage/blobs/proxy/:signed_id/*filename(.:format)
Controller#Action | active_storage/blobs/proxy#show
--[ Route 18 ]-------------
Prefix |
Prefix |#{" "}
Verb | GET
URI | /rails/active_storage/blobs/:signed_id/*filename(.:format)
Controller#Action | active_storage/blobs/redirect#show
Expand All @@ -311,7 +311,7 @@ class Rails::Command::RoutesTest < ActiveSupport::TestCase
URI | /rails/active_storage/representations/proxy/:signed_blob_id/:variation_key/*filename(.:format)
Controller#Action | active_storage/representations/proxy#show
--[ Route 21 ]-------------
Prefix |
Prefix |#{" "}
Verb | GET
URI | /rails/active_storage/representations/:signed_blob_id/:variation_key/*filename(.:format)
Controller#Action | active_storage/representations/redirect#show
Expand All @@ -334,6 +334,17 @@ class Rails::Command::RoutesTest < ActiveSupport::TestCase
# rubocop:enable Layout/TrailingWhitespace
end

test "rails routes with unused option" do
app_file "config/routes.rb", <<-RUBY
Rails.application.routes.draw do
end
RUBY

output = run_routes_command([ "--unused" ])

assert_equal(output, "No unused routes found.\n")
end

private
def run_routes_command(args = [])
rails "routes", args
Expand Down
154 changes: 154 additions & 0 deletions railties/test/commands/unused_routes_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,154 @@
# frozen_string_literal: true

require "isolation/abstract_unit"
require "rails/command"
require "rails/commands/routes/routes_command"
require "io/console/size"

class Rails::Command::UnusedRoutesTest < ActiveSupport::TestCase
setup :build_app
teardown :teardown_app

test "no results" do
app_file "config/routes.rb", <<-RUBY
Rails.application.routes.draw do
end
RUBY

assert_equal <<~OUTPUT, run_unused_routes_command
No unused routes found.
OUTPUT
end

test "no controller" do
app_file "config/routes.rb", <<-RUBY
Rails.application.routes.draw do
get "/", to: "my#index", as: :my_route
end
RUBY

assert_equal <<~OUTPUT, run_unused_routes_command(allow_failure: true)
Found 1 unused route:

Prefix Verb URI Pattern Controller#Action
my_route GET / my#index
OUTPUT
end

test "no action" do
app_file "app/controllers/my_controller.rb", <<-RUBY
class MyController < ActionController::Base
end
RUBY

app_file "config/routes.rb", <<-RUBY
Rails.application.routes.draw do
get "/", to: "my#index", as: :my_route
end
RUBY

assert_equal <<~OUTPUT, run_unused_routes_command(allow_failure: true)
Found 1 unused route:

Prefix Verb URI Pattern Controller#Action
my_route GET / my#index
OUTPUT
end

test "implicit render" do
app_file "app/controllers/my_controller.rb", <<-RUBY
class MyController < ActionController::Base
end
RUBY

app_file "app/views/my/index.html.erb", <<-HTML
<h1>Hello world</h1>
HTML

app_file "config/routes.rb", <<-RUBY
Rails.application.routes.draw do
get "/", to: "my#index", as: :my_route
end
RUBY

assert_equal <<~OUTPUT, run_unused_routes_command
No unused routes found.
OUTPUT
end

test "multiple unused routes" do
app_file "config/routes.rb", <<-RUBY
Rails.application.routes.draw do
get "/one", to: "action#one"
get "/two", to: "action#two"
end
RUBY

assert_equal <<~OUTPUT, run_unused_routes_command(allow_failure: true)
Found 2 unused routes:

Prefix Verb URI Pattern Controller#Action
one GET /one(.:format) action#one
two GET /two(.:format) action#two
OUTPUT
end

test "filter by grep" do
app_file "config/routes.rb", <<-RUBY
Rails.application.routes.draw do
get "/one", to: "posts#one"
get "/two", to: "users#two"
end
RUBY

assert_equal <<~OUTPUT, run_unused_routes_command(["-g", "one"], allow_failure: true)
Found 1 unused route:

Prefix Verb URI Pattern Controller#Action
one GET /one(.:format) posts#one
OUTPUT
end

test "filter by grep no results" do
app_file "config/routes.rb", <<-RUBY
Rails.application.routes.draw do
end
RUBY

assert_equal <<~OUTPUT, run_unused_routes_command(["-g", "one"])
No unused routes found for this grep pattern.
OUTPUT
end

test "filter by controller" do
app_file "config/routes.rb", <<-RUBY
Rails.application.routes.draw do
get "/one", to: "posts#one"
get "/two", to: "users#two"
end
RUBY

assert_equal <<~OUTPUT, run_unused_routes_command(["-c", "posts"], allow_failure: true)
Found 1 unused route:

Prefix Verb URI Pattern Controller#Action
one GET /one(.:format) posts#one
OUTPUT
end

test "filter by controller no results" do
app_file "config/routes.rb", <<-RUBY
Rails.application.routes.draw do
end
RUBY

assert_equal <<~OUTPUT, run_unused_routes_command(["-c", "posts"])
No unused routes found for this controller.
OUTPUT
end

private
def run_unused_routes_command(args = [], allow_failure: false)
rails "unused_routes", args, allow_failure: allow_failure
end
end