From ee057d28ad2af6e1645665aac7ed79bf65577dd8 Mon Sep 17 00:00:00 2001 From: fatkodima Date: Sun, 28 Jun 2020 16:13:55 +0300 Subject: [PATCH] Add new `Rails/MatchRoute` cop --- CHANGELOG.md | 1 + config/default.yml | 11 ++ docs/modules/ROOT/pages/cops.adoc | 1 + docs/modules/ROOT/pages/cops_rails.adoc | 47 +++++++++ lib/rubocop/cop/rails/match_route.rb | 117 +++++++++++++++++++++ lib/rubocop/cop/rails_cops.rb | 1 + spec/rubocop/cop/rails/match_route_spec.rb | 95 +++++++++++++++++ 7 files changed, 273 insertions(+) create mode 100644 lib/rubocop/cop/rails/match_route.rb create mode 100644 spec/rubocop/cop/rails/match_route_spec.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index eaa6b11ae8..9785ca07c8 100644 --- a/CHANGELOG.md +++ b/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][]) diff --git a/config/default.yml b/config/default.yml index 88ab9e12d6..b49e3b61f8 100644 --- a/config/default.yml +++ b/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' diff --git a/docs/modules/ROOT/pages/cops.adoc b/docs/modules/ROOT/pages/cops.adoc index 69915512d3..00ab66c562 100644 --- a/docs/modules/ROOT/pages/cops.adoc +++ b/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] diff --git a/docs/modules/ROOT/pages/cops_rails.adoc b/docs/modules/ROOT/pages/cops_rails.adoc index 96095f8eae..0419a89d13 100644 --- a/docs/modules/ROOT/pages/cops_rails.adoc +++ b/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 |=== diff --git a/lib/rubocop/cop/rails/match_route.rb b/lib/rubocop/cop/rails/match_route.rb new file mode 100644 index 0000000000..01b7455fa8 --- /dev/null +++ b/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 `%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 diff --git a/lib/rubocop/cop/rails_cops.rb b/lib/rubocop/cop/rails_cops.rb index a999a8142a..769246fbef 100644 --- a/lib/rubocop/cop/rails_cops.rb +++ b/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' diff --git a/spec/rubocop/cop/rails/match_route_spec.rb b/spec/rubocop/cop/rails/match_route_spec.rb new file mode 100644 index 0000000000..fc5bd1c541 --- /dev/null +++ b/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