Skip to content

Commit

Permalink
Add new Rails/FindById cop
Browse files Browse the repository at this point in the history
  • Loading branch information
fatkodima committed Jun 30, 2020
1 parent 840f152 commit e427e99
Show file tree
Hide file tree
Showing 7 changed files with 191 additions and 0 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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][])
Expand Down
8 changes: 8 additions & 0 deletions config/default.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down
1 change: 1 addition & 0 deletions docs/modules/ROOT/pages/cops.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down
33 changes: 33 additions & 0 deletions docs/modules/ROOT/pages/cops_rails.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -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

|===
Expand Down
103 changes: 103 additions & 0 deletions lib/rubocop/cop/rails/find_by_id.rb
Original file line number Diff line number Diff line change
@@ -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
1 change: 1 addition & 0 deletions lib/rubocop/cop/rails_cops.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down
44 changes: 44 additions & 0 deletions spec/rubocop/cop/rails/find_by_id_spec.rb
Original file line number Diff line number Diff line change
@@ -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

0 comments on commit e427e99

Please sign in to comment.