From 8cb2bddaef798bf3d966434156b0a9e9f230ca4e Mon Sep 17 00:00:00 2001 From: Koichi ITO Date: Mon, 6 Jan 2025 18:57:07 +0900 Subject: [PATCH] Add new `Rails/MultipleRoutePaths` cop 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 https://github.com/rails/rails/pull/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. ... ``` --- ..._add_new_rails_multiple_route_paths_cop.md | 1 + config/default.yml | 6 + lib/rubocop/cop/mixin/routes_helper.rb | 20 ++++ lib/rubocop/cop/rails/match_route.rb | 10 +- lib/rubocop/cop/rails/multiple_route_paths.rb | 50 ++++++++ lib/rubocop/cop/rails_cops.rb | 2 + .../cop/rails/multiple_route_paths_spec.rb | 112 ++++++++++++++++++ 7 files changed, 192 insertions(+), 9 deletions(-) create mode 100644 changelog/new_add_new_rails_multiple_route_paths_cop.md create mode 100644 lib/rubocop/cop/mixin/routes_helper.rb create mode 100644 lib/rubocop/cop/rails/multiple_route_paths.rb create mode 100644 spec/rubocop/cop/rails/multiple_route_paths_spec.rb diff --git a/changelog/new_add_new_rails_multiple_route_paths_cop.md b/changelog/new_add_new_rails_multiple_route_paths_cop.md new file mode 100644 index 0000000000..44dbbf47ee --- /dev/null +++ b/changelog/new_add_new_rails_multiple_route_paths_cop.md @@ -0,0 +1 @@ +* [#1407](https://github.com/rubocop/rubocop-rails/pull/1407): Add new `Rails/MultipleRoutePaths` cop. ([@koic][]) diff --git a/config/default.yml b/config/default.yml index 7e64608a9c..22c7edb9d4 100644 --- a/config/default.yml +++ b/config/default.yml @@ -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: '<>' + Rails/NegateInclude: Description: 'Prefer `collection.exclude?(obj)` over `!collection.include?(obj)`.' StyleGuide: 'https://rails.rubystyle.guide#exclude' diff --git a/lib/rubocop/cop/mixin/routes_helper.rb b/lib/rubocop/cop/mixin/routes_helper.rb new file mode 100644 index 0000000000..0446e82d6e --- /dev/null +++ b/lib/rubocop/cop/mixin/routes_helper.rb @@ -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 diff --git a/lib/rubocop/cop/rails/match_route.rb b/lib/rubocop/cop/rails/match_route.rb index e7c7b4c418..27d7d7b917 100644 --- a/lib/rubocop/cop/rails/match_route.rb +++ b/lib/rubocop/cop/rails/match_route.rb @@ -21,11 +21,11 @@ module Rails # match 'photos/:id', to: 'photos#show', via: :all # class MatchRoute < Base + include RoutesHelper extend AutoCorrector MSG = 'Use `%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 ...) ?) @@ -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 diff --git a/lib/rubocop/cop/rails/multiple_route_paths.rb b/lib/rubocop/cop/rails/multiple_route_paths.rb new file mode 100644 index 0000000000..31bbb340d0 --- /dev/null +++ b/lib/rubocop/cop/rails/multiple_route_paths.rb @@ -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 diff --git a/lib/rubocop/cop/rails_cops.rb b/lib/rubocop/cop/rails_cops.rb index b7eddc4fe4..047fd87bde 100644 --- a/lib/rubocop/cop/rails_cops.rb +++ b/lib/rubocop/cop/rails_cops.rb @@ -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' @@ -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' diff --git a/spec/rubocop/cop/rails/multiple_route_paths_spec.rb b/spec/rubocop/cop/rails/multiple_route_paths_spec.rb new file mode 100644 index 0000000000..3e0227c227 --- /dev/null +++ b/spec/rubocop/cop/rails/multiple_route_paths_spec.rb @@ -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