Add new Rails/FindById cop

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][])

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'

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]

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
 
 |===

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 `%<good_method>s` instead of `%<bad_method>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

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'

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