Skip to content

Commit

Permalink
Add new Rails/MultipleRoutePaths cop
Browse files Browse the repository at this point in the history
This PR adds new `Rails/MultipleRoutePaths` cop that
checks if mapping a route with multiple paths is deprecated and will be removed in Rails 8.1.

```ruby
# bad
get '/users', '/other_path', to: 'users#index'

# good
get '/users', to: 'users#index'
get '/other_path', to: 'users#index'
```

This provides a migration path for the following warning added in rails/rails#52409.

```consle
DEPRECATION WARNING: Mapping a route with multiple paths is deprecated and will be removed in Rails 8.1.
Please use multiple method calls instead. ...
```
  • Loading branch information
koic committed Jan 8, 2025
1 parent 1126163 commit 8cb2bdd
Show file tree
Hide file tree
Showing 7 changed files with 192 additions and 9 deletions.
1 change: 1 addition & 0 deletions changelog/new_add_new_rails_multiple_route_paths_cop.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
* [#1407](https://github.com/rubocop/rubocop-rails/pull/1407): Add new `Rails/MultipleRoutePaths` cop. ([@koic][])
6 changes: 6 additions & 0 deletions config/default.yml
Original file line number Diff line number Diff line change
Expand Up @@ -698,6 +698,12 @@ Rails/MigrationClassName:
Include:
- db/**/*.rb

Rails/MultipleRoutePaths:
Description: 'Checks if mapping a route with multiple paths is deprecated and will be removed in Rails 8.1.'
Enabled: pending
Severity: warning
VersionAdded: '<<next>>'

Rails/NegateInclude:
Description: 'Prefer `collection.exclude?(obj)` over `!collection.include?(obj)`.'
StyleGuide: 'https://rails.rubystyle.guide#exclude'
Expand Down
20 changes: 20 additions & 0 deletions lib/rubocop/cop/mixin/routes_helper.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
# frozen_string_literal: true

module RuboCop
module Cop
# Common functionality for cops working with routes.
module RoutesHelper
extend NodePattern::Macros

HTTP_METHODS = %i[get post put patch delete].freeze

def_node_matcher :routes_draw?, <<~PATTERN
(send (send _ :routes) :draw)
PATTERN

def within_routes?(node)
node.each_ancestor(:block).any? { |block| routes_draw?(block.send_node) }
end
end
end
end
10 changes: 1 addition & 9 deletions lib/rubocop/cop/rails/match_route.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,11 @@ module Rails
# match 'photos/:id', to: 'photos#show', via: :all
#
class MatchRoute < Base
include RoutesHelper
extend AutoCorrector

MSG = 'Use `%<http_method>s` instead of `match` to define a route.'
RESTRICT_ON_SEND = %i[match].freeze
HTTP_METHODS = %i[get post put patch delete].freeze

def_node_matcher :match_method_call?, <<~PATTERN
(send nil? :match $_ $(hash ...) ?)
Expand Down Expand Up @@ -60,14 +60,6 @@ def register_offense(node, http_method)
end
end

def_node_matcher :routes_draw?, <<~PATTERN
(send (send _ :routes) :draw)
PATTERN

def within_routes?(node)
node.each_ancestor(:block).any? { |a| routes_draw?(a.send_node) }
end

def extract_via(node)
via_pair = via_pair(node)
return %i[get] unless via_pair
Expand Down
50 changes: 50 additions & 0 deletions lib/rubocop/cop/rails/multiple_route_paths.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
# frozen_string_literal: true

module RuboCop
module Cop
module Rails
# Checks if mapping a route with multiple paths is deprecated and will be removed in Rails 8.1.
#
# @example
#
# # bad
# get '/users', '/other_path', to: 'users#index'
#
# # good
# get '/users', to: 'users#index'
# get '/other_path', to: 'users#index'
#
class MultipleRoutePaths < Base
include RoutesHelper
extend AutoCorrector

MSG = 'Use separate routes instead of combining multiple route paths in a single route.'
RESTRICT_ON_SEND = HTTP_METHODS

IGNORED_ARGUMENT_TYPES = %i[array hash].freeze

def on_send(node)
return unless within_routes?(node)

route_paths = node.arguments.reject { |argument| IGNORED_ARGUMENT_TYPES.include?(argument.type) }
return if route_paths.count < 2

add_offense(node) do |corrector|
corrector.replace(node, migrate_to_multiple_routes(node, route_paths))
end
end

private

def migrate_to_multiple_routes(node, route_paths)
rest = route_paths.last.source_range.end.join(node.source_range.end).source
indentation = ' ' * node.source_range.column

route_paths.map do |route_path|
"#{node.method_name} #{route_path.source}#{rest}"
end.join("\n#{indentation}")
end
end
end
end
end
2 changes: 2 additions & 0 deletions lib/rubocop/cop/rails_cops.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
require_relative 'mixin/enforce_superclass'
require_relative 'mixin/index_method'
require_relative 'mixin/migrations_helper'
require_relative 'mixin/routes_helper'
require_relative 'mixin/target_rails_version'

require_relative 'rails/action_controller_flash_before_render'
Expand Down Expand Up @@ -77,6 +78,7 @@
require_relative 'rails/mailer_name'
require_relative 'rails/match_route'
require_relative 'rails/migration_class_name'
require_relative 'rails/multiple_route_paths'
require_relative 'rails/negate_include'
require_relative 'rails/not_null_column'
require_relative 'rails/order_by_id'
Expand Down
112 changes: 112 additions & 0 deletions spec/rubocop/cop/rails/multiple_route_paths_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
# frozen_string_literal: true

RSpec.describe RuboCop::Cop::Rails::MultipleRoutePaths, :config do
it 'registers an offense when using a route with multiple string route paths' do
expect_offense(<<~RUBY)
Rails.application.routes.draw do
get '/users', '/other_path/users', '/another_path/users'
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use separate routes instead of combining multiple route paths in a single route.
end
RUBY

expect_correction(<<~RUBY)
Rails.application.routes.draw do
get '/users'
get '/other_path/users'
get '/another_path/users'
end
RUBY
end

it 'registers an offense when using a route with multiple route paths with option' do
expect_offense(<<~RUBY)
Rails.application.routes.draw do
get '/users', '/other_path/users', '/another_path/users', to: 'users#index'
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use separate routes instead of combining multiple route paths in a single route.
end
RUBY

expect_correction(<<~RUBY)
Rails.application.routes.draw do
get '/users', to: 'users#index'
get '/other_path/users', to: 'users#index'
get '/another_path/users', to: 'users#index'
end
RUBY
end

it 'registers an offense when using a route with multiple route paths with splat option' do
expect_offense(<<~RUBY)
Rails.application.routes.draw do
get '/users', '/other_path/users', '/another_path/users', **options
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use separate routes instead of combining multiple route paths in a single route.
end
RUBY

expect_correction(<<~RUBY)
Rails.application.routes.draw do
get '/users', **options
get '/other_path/users', **options
get '/another_path/users', **options
end
RUBY
end

it 'registers an offense when using a route with multiple symbol route paths' do
expect_offense(<<~RUBY)
Rails.application.routes.draw do
get :resend, :generate_new_password
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use separate routes instead of combining multiple route paths in a single route.
end
RUBY

expect_correction(<<~RUBY)
Rails.application.routes.draw do
get :resend
get :generate_new_password
end
RUBY
end

it 'does not register an offense when using single string path method calls' do
expect_no_offenses(<<~RUBY)
Rails.application.routes.draw do
get '/users'
get '/other_path/users'
get '/another_path/users'
end
RUBY
end

it 'does not register an offense when using single string path with option method calls' do
expect_no_offenses(<<~RUBY)
Rails.application.routes.draw do
get '/users', to: 'users#index'
get '/other_path/users', to: 'users#index'
get '/another_path/users', to: 'users#index'
end
RUBY
end

it 'does not register an offense when using single string path with array literal' do
expect_no_offenses(<<~RUBY)
Rails.application.routes.draw do
get '/other_path/users', []
end
RUBY
end

it 'does not register an offense when using single route path with no arguments' do
expect_no_offenses(<<~RUBY)
Rails.application.routes.draw do
get
end
RUBY
end

it 'does not register an offense when not within routes' do
expect_no_offenses(<<~RUBY)
get '/users', '/other_path/users', '/another_path/users'
RUBY
end
end

0 comments on commit 8cb2bdd

Please sign in to comment.