Merge pull request #275 from fatkodima/match-route-cop

Add new `Rails/MatchRoute` cop

CHANGELOG.md

@@ -4,6 +4,7 @@
 
 ### New features
 
+* [#275](https://github.com/rubocop-hq/rubocop-rails/pull/275): Add new `Rails/MatchRoute` cop. ([@fatkodima][])
 * [#271](https://github.com/rubocop-hq/rubocop-rails/pull/271): Add new `Rails/RenderInline` cop. ([@fatkodima][])
 * [#281](https://github.com/rubocop-hq/rubocop-rails/pull/281): Add new `Rails/MailerName` cop. ([@fatkodima][])
 * [#246](https://github.com/rubocop-hq/rubocop-rails/issues/246): Add new `Rails/PluckInWhere` cop. ([@fatkodima][])

config/default.yml

@@ -333,6 +333,17 @@ Rails/MailerName:
   Include:
     - app/mailers/**/*.rb
 
+Rails/MatchRoute:
+  Description: >-
+                  Don't use `match` to define any routes unless there is a need to map multiple request types
+                  among [:get, :post, :patch, :put, :delete] to a single action using the `:via` option.
+  StyleGuide: 'https://rails.rubystyle.guide/#no-match-routes'
+  Enabled: 'pending'
+  VersionAdded: '2.7'
+  Include:
+    - config/routes.rb
+    - config/routes/**/*.rb
+
 Rails/NegateInclude:
   Description: 'Prefer `collection.exclude?(obj)` over `!collection.include?(obj)`.'
   Enabled: 'pending'

docs/modules/ROOT/pages/cops.adoc

@@ -39,6 +39,7 @@
 * xref:cops_rails.adoc#railslexicallyscopedactionfilter[Rails/LexicallyScopedActionFilter]
 * xref:cops_rails.adoc#railslinktoblank[Rails/LinkToBlank]
 * xref:cops_rails.adoc#railsmailername[Rails/MailerName]
+* xref:cops_rails.adoc#railsmatchroute[Rails/MatchRoute]
 * xref:cops_rails.adoc#railsnegateinclude[Rails/NegateInclude]
 * xref:cops_rails.adoc#railsnotnullcolumn[Rails/NotNullColumn]
 * xref:cops_rails.adoc#railsoutput[Rails/Output]

docs/modules/ROOT/pages/cops_rails.adoc

@@ -1944,6 +1944,53 @@ end
 
 * https://rails.rubystyle.guide/#mailer-name
 
+== Rails/MatchRoute
+
+|===
+| Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged
+
+| Pending
+| Yes
+| Yes
+| 2.7
+| -
+|===
+
+This cop identifies places where defining routes with `match`
+can be replaced with a specific HTTP method.
+
+Don't use `match` to define any routes unless there is a need to map multiple request types
+among [:get, :post, :patch, :put, :delete] to a single action using the `:via` option.
+
+=== Examples
+
+[source,ruby]
+----
+# bad
+match ':controller/:action/:id'
+match 'photos/:id', to: 'photos#show', via: :get
+
+# good
+get ':controller/:action/:id'
+get 'photos/:id', to: 'photos#show'
+match 'photos/:id', to: 'photos#show', via: [:get, :post]
+match 'photos/:id', to: 'photos#show', via: :all
+----
+
+=== Configurable attributes
+
+|===
+| Name | Default value | Configurable values
+
+| Include
+| `config/routes.rb`, `config/routes/**/*.rb`
+| Array
+|===
+
+=== References
+
+* https://rails.rubystyle.guide/#no-match-routes
+
 == Rails/NegateInclude
 
 |===

lib/rubocop/cop/rails/match_route.rb

@@ -0,0 +1,117 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module Rails
+      # This cop identifies places where defining routes with `match`
+      # can be replaced with a specific HTTP method.
+      #
+      # Don't use `match` to define any routes unless there is a need to map multiple request types
+      # among [:get, :post, :patch, :put, :delete] to a single action using the `:via` option.
+      #
+      # @example
+      #   # bad
+      #   match ':controller/:action/:id'
+      #   match 'photos/:id', to: 'photos#show', via: :get
+      #
+      #   # good
+      #   get ':controller/:action/:id'
+      #   get 'photos/:id', to: 'photos#show'
+      #   match 'photos/:id', to: 'photos#show', via: [:get, :post]
+      #   match 'photos/:id', to: 'photos#show', via: :all
+      #
+      class MatchRoute < Cop
+        MSG = 'Use `%<http_method>s` instead of `match` to define a route.'
+        HTTP_METHODS = %i[get post put patch delete].freeze
+
+        def_node_matcher :match_method_call?, <<~PATTERN
+          (send nil? :match $_ $(hash ...) ?)
+        PATTERN
+
+        def on_send(node)
+          match_method_call?(node) do |path_node, options_node|
+            return unless within_routes?(node)
+
+            options_node = path_node.hash_type? ? path_node : options_node.first
+
+            if options_node.nil?
+              message = format(MSG, http_method: 'get')
+              add_offense(node, message: message)
+            else
+              via = extract_via(options_node)
+              if via.size == 1 && http_method?(via.first)
+                message = format(MSG, http_method: via.first)
+                add_offense(node, message: message)
+              end
+            end
+          end
+        end
+
+        def autocorrect(node)
+          match_method_call?(node) do |path_node, options_node|
+            options_node = options_node.first
+
+            lambda do |corrector|
+              corrector.replace(node, replacement(path_node, options_node))
+            end
+          end
+        end
+
+        private
+
+        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
+
+          _, via = *via_pair
+
+          if via.basic_literal?
+            [via.value]
+          elsif via.array_type?
+            via.values.map(&:value)
+          end
+        end
+
+        def via_pair(node)
+          node.pairs.find { |p| p.key.value == :via }
+        end
+
+        def http_method?(method)
+          HTTP_METHODS.include?(method.to_sym)
+        end
+
+        def replacement(path_node, options_node)
+          if path_node.hash_type?
+            http_method, options = *http_method_and_options(path_node)
+            "#{http_method} #{options.map(&:source).join(', ')}"
+          elsif options_node.nil?
+            "get #{path_node.source}"
+          else
+            http_method, options = *http_method_and_options(options_node)
+
+            if options.any?
+              "#{http_method} #{path_node.source}, #{options.map(&:source).join(', ')}"
+            else
+              "#{http_method} #{path_node.source}"
+            end
+          end
+        end
+
+        def http_method_and_options(node)
+          via_pair = via_pair(node)
+          http_method = extract_via(node).first
+          rest_pairs = node.pairs - [via_pair]
+          [http_method, rest_pairs]
+        end
+      end
+    end
+  end
+end

lib/rubocop/cop/rails_cops.rb

@@ -41,6 +41,7 @@
 require_relative 'rails/lexically_scoped_action_filter'
 require_relative 'rails/link_to_blank'
 require_relative 'rails/mailer_name'
+require_relative 'rails/match_route'
 require_relative 'rails/negate_include'
 require_relative 'rails/not_null_column'
 require_relative 'rails/output'

spec/rubocop/cop/rails/match_route_spec.rb

@@ -0,0 +1,95 @@
+# frozen_string_literal: true
+
+RSpec.describe RuboCop::Cop::Rails::MatchRoute do
+  subject(:cop) { described_class.new }
+
+  it 'registers an offense and corrects when using `match` only with path' do
+    expect_offense(<<~RUBY)
+      routes.draw do
+        match ':controller/:action/:id'
+        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `get` instead of `match` to define a route.
+      end
+    RUBY
+
+    expect_correction(<<~RUBY)
+      routes.draw do
+        get ':controller/:action/:id'
+      end
+    RUBY
+  end
+
+  it 'registers an offense and corrects when using `match` with single :via' do
+    expect_offense(<<~RUBY)
+      routes.draw do
+        match 'photos/:id', to: 'photos#show', via: :get
+        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `get` instead of `match` to define a route.
+      end
+    RUBY
+
+    expect_correction(<<~RUBY)
+      routes.draw do
+        get 'photos/:id', to: 'photos#show'
+      end
+    RUBY
+  end
+
+  it 'registers an offense and corrects when using `match` with one item in :via array' do
+    expect_offense(<<~RUBY)
+      routes.draw do
+        match 'photos/:id', to: 'photos#show', via: [:get]
+        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `get` instead of `match` to define a route.
+      end
+    RUBY
+
+    expect_correction(<<~RUBY)
+      routes.draw do
+        get 'photos/:id', to: 'photos#show'
+      end
+    RUBY
+  end
+
+  it 'registers an offense and corrects when using `match` with hash shorthand' do
+    expect_offense(<<~RUBY)
+      routes.draw do
+        match 'photos/:id' => 'photos#show', via: :get
+        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `get` instead of `match` to define a route.
+      end
+    RUBY
+
+    expect_correction(<<~RUBY)
+      routes.draw do
+        get 'photos/:id' => 'photos#show'
+      end
+    RUBY
+  end
+
+  it 'does not register an offense when not within routes' do
+    expect_no_offenses(<<~RUBY)
+      match 'photos/:id', to: 'photos#show', via: :get
+    RUBY
+  end
+
+  it 'does not register an offense when using `match` with `via: :all`' do
+    expect_no_offenses(<<~RUBY)
+      routes.draw do
+        match 'photos/:id', to: 'photos#show', via: :all
+      end
+    RUBY
+  end
+
+  it 'does not register an offense when using `match` with multiple verbs in :via array' do
+    expect_no_offenses(<<~RUBY)
+      routes.draw do
+        match 'photos/:id', to: 'photos#update', via: [:put, :patch]
+      end
+    RUBY
+  end
+
+  it 'does not register an offense when using `get`' do
+    expect_no_offenses(<<~RUBY)
+      routes.draw do
+        get 'photos/:id', to: 'photos#show'
+      end
+    RUBY
+  end
+end