Add new Rails/ActiveRecordFind cop

CHANGELOG.md

@@ -4,6 +4,7 @@
 
 ### New features
 
+* [#283](https://github.com/rubocop-hq/rubocop-rails/pull/283): Add new `Rails/ActiveRecordFind` 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

@@ -37,6 +37,14 @@ Rails/ActiveRecordAliases:
   VersionAdded: '0.53'
   SafeAutoCorrect: false
 
+Rails/ActiveRecordFind:
+  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/ActiveRecordOverride:
   Description: >-
                   Check for overriding Active Record methods instead of using

docs/modules/ROOT/pages/cops.adoc

@@ -4,6 +4,7 @@
 
 * xref:cops_rails.adoc#railsactionfilter[Rails/ActionFilter]
 * xref:cops_rails.adoc#railsactiverecordaliases[Rails/ActiveRecordAliases]
+* xref:cops_rails.adoc#railsactiverecordfind[Rails/ActiveRecordFind]
 * xref:cops_rails.adoc#railsactiverecordoverride[Rails/ActiveRecordOverride]
 * xref:cops_rails.adoc#railsactivesupportaliases[Rails/ActiveSupportAliases]
 * xref:cops_rails.adoc#railsapplicationcontroller[Rails/ApplicationController]

docs/modules/ROOT/pages/cops_rails.adoc

@@ -89,6 +89,39 @@ Book.update_attributes!(author: 'Alice')
 Book.update!(author: 'Alice')
 ----
 
+== Rails/ActiveRecordFind
+
+|===
+| 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/ActiveRecordOverride
 
 |===

lib/rubocop/cop/rails/active_record_find.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 ActiveRecordFind < 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

@@ -6,6 +6,7 @@
 
 require_relative 'rails/action_filter'
 require_relative 'rails/active_record_aliases'
+require_relative 'rails/active_record_find'
 require_relative 'rails/active_record_override'
 require_relative 'rails/active_support_aliases'
 require_relative 'rails/application_controller'

spec/rubocop/cop/rails/active_record_find_spec.rb

@@ -0,0 +1,44 @@
+# frozen_string_literal: true
+
+RSpec.describe RuboCop::Cop::Rails::ActiveRecordFind 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