diff --git a/CHANGELOG.md b/CHANGELOG.md index 9785ca07c8..e19fc115c1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ ### New features +* [#283](https://github.com/rubocop-hq/rubocop-rails/pull/283): Add new `Rails/FindById` cop. ([@fatkodima][]) * [#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][]) diff --git a/config/default.yml b/config/default.yml index b49e3b61f8..2daec271e3 100644 --- a/config/default.yml +++ b/config/default.yml @@ -234,6 +234,14 @@ Rails/FindBy: Include: - app/models/**/*.rb +Rails/FindById: + Description: >- + Favor the use of `find` over `where.take!`, `find_by!`, and `find_by_id!` when you + need to retrieve a single record by primary key when you expect it to be found. + StyleGuide: 'https://rails.rubystyle.guide/#find' + Enabled: 'pending' + VersionAdded: '2.7' + Rails/FindEach: Description: 'Prefer all.find_each over all.find.' StyleGuide: 'https://rails.rubystyle.guide#find-each' diff --git a/docs/modules/ROOT/pages/cops.adoc b/docs/modules/ROOT/pages/cops.adoc index 00ab66c562..c71c433060 100644 --- a/docs/modules/ROOT/pages/cops.adoc +++ b/docs/modules/ROOT/pages/cops.adoc @@ -26,6 +26,7 @@ * xref:cops_rails.adoc#railsexit[Rails/Exit] * xref:cops_rails.adoc#railsfilepath[Rails/FilePath] * xref:cops_rails.adoc#railsfindby[Rails/FindBy] +* xref:cops_rails.adoc#railsfindbyid[Rails/FindById] * xref:cops_rails.adoc#railsfindeach[Rails/FindEach] * xref:cops_rails.adoc#railshasandbelongstomany[Rails/HasAndBelongsToMany] * xref:cops_rails.adoc#railshasmanyorhasonedependent[Rails/HasManyOrHasOneDependent] diff --git a/docs/modules/ROOT/pages/cops_rails.adoc b/docs/modules/ROOT/pages/cops_rails.adoc index 0419a89d13..53e2e2f7d6 100644 --- a/docs/modules/ROOT/pages/cops_rails.adoc +++ b/docs/modules/ROOT/pages/cops_rails.adoc @@ -1186,6 +1186,39 @@ User.find_by(name: 'Bruce') * https://rails.rubystyle.guide#find_by +== Rails/FindById + +|=== +| Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged + +| Pending +| Yes +| Yes +| 2.7 +| - +|=== + +This cop enforces that `ActiveRecord#find` is used instead of +`where.take!`, `find_by!`, and `find_by_id!` to retrieve a single record +by primary key when you expect it to be found. + +=== Examples + +[source,ruby] +---- +# bad +User.where(id: id).take! +User.find_by_id!(id) +User.find_by!(id: id) + +# good +User.find(id) +---- + +=== References + +* https://rails.rubystyle.guide/#find + == Rails/FindEach |=== diff --git a/lib/rubocop/cop/rails/find_by_id.rb b/lib/rubocop/cop/rails/find_by_id.rb new file mode 100644 index 0000000000..7d7837ec78 --- /dev/null +++ b/lib/rubocop/cop/rails/find_by_id.rb @@ -0,0 +1,103 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Rails + # This cop enforces that `ActiveRecord#find` is used instead of + # `where.take!`, `find_by!`, and `find_by_id!` to retrieve a single record + # by primary key when you expect it to be found. + # + # @example + # # bad + # User.where(id: id).take! + # User.find_by_id!(id) + # User.find_by!(id: id) + # + # # good + # User.find(id) + # + class FindById < Cop + include RangeHelp + + MSG = 'Use `%s` instead of `%s`.' + + def_node_matcher :where_take?, <<~PATTERN + (send + $(send _ :where + (hash + (pair (sym :id) $_))) :take!) + PATTERN + + def_node_matcher :find_by?, <<~PATTERN + { + (send _ :find_by_id! $_) + (send _ :find_by! (hash (pair (sym :id) $_))) + } + PATTERN + + def on_send(node) + where_take?(node) do |where, id_value| + range = where_take_offense_range(node, where) + + good_method = build_good_method(id_value) + bad_method = build_where_take_bad_method(id_value) + message = format(MSG, good_method: good_method, bad_method: bad_method) + + add_offense(node, location: range, message: message) + end + + find_by?(node) do |id_value| + range = find_by_offense_range(node) + + good_method = build_good_method(id_value) + bad_method = build_find_by_bad_method(node, id_value) + message = format(MSG, good_method: good_method, bad_method: bad_method) + + add_offense(node, location: range, message: message) + end + end + + def autocorrect(node) + if (matches = where_take?(node)) + where, id_value = *matches + range = where_take_offense_range(node, where) + elsif (id_value = find_by?(node)) + range = find_by_offense_range(node) + end + + lambda do |corrector| + replacement = build_good_method(id_value) + corrector.replace(range, replacement) + end + end + + private + + def where_take_offense_range(node, where) + range_between(where.loc.selector.begin_pos, node.loc.expression.end_pos) + end + + def find_by_offense_range(node) + range_between(node.loc.selector.begin_pos, node.loc.expression.end_pos) + end + + def build_good_method(id_value) + "find(#{id_value.source})" + end + + def build_where_take_bad_method(id_value) + "where(id: #{id_value.source}).take!" + end + + def build_find_by_bad_method(node, id_value) + case node.method_name + when :find_by_id! + "find_by_id!(#{id_value.source})" + when :find_by! + "find_by!(id: #{id_value.source})" + end + end + end + end + end +end diff --git a/lib/rubocop/cop/rails_cops.rb b/lib/rubocop/cop/rails_cops.rb index 769246fbef..b24259f0b9 100644 --- a/lib/rubocop/cop/rails_cops.rb +++ b/lib/rubocop/cop/rails_cops.rb @@ -28,6 +28,7 @@ require_relative 'rails/exit' require_relative 'rails/file_path' require_relative 'rails/find_by' +require_relative 'rails/find_by_id' require_relative 'rails/find_each' require_relative 'rails/has_and_belongs_to_many' require_relative 'rails/has_many_or_has_one_dependent' diff --git a/spec/rubocop/cop/rails/find_by_id_spec.rb b/spec/rubocop/cop/rails/find_by_id_spec.rb new file mode 100644 index 0000000000..35469aef54 --- /dev/null +++ b/spec/rubocop/cop/rails/find_by_id_spec.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::Rails::FindById do + subject(:cop) { described_class.new } + + it 'registers an offense and corrects when using `where(id: ...).take!`' do + expect_offense(<<~RUBY) + User.where(id: 1).take! + ^^^^^^^^^^^^^^^^^^ Use `find(1)` instead of `where(id: 1).take!`. + RUBY + + expect_correction(<<~RUBY) + User.find(1) + RUBY + end + + it 'registers an offense and corrects when using `find_by_id!`' do + expect_offense(<<~RUBY) + User.find_by_id!(1) + ^^^^^^^^^^^^^^ Use `find(1)` instead of `find_by_id!(1)`. + RUBY + + expect_correction(<<~RUBY) + User.find(1) + RUBY + end + + it 'registers an offense and corrects when using `find_by!(id: ...)`' do + expect_offense(<<~RUBY) + User.find_by!(id: 1) + ^^^^^^^^^^^^^^^ Use `find(1)` instead of `find_by!(id: 1)`. + RUBY + + expect_correction(<<~RUBY) + User.find(1) + RUBY + end + + it 'does not register an offense when using `find`' do + expect_no_offenses(<<~RUBY) + User.find(1) + RUBY + end +end