diff --git a/CHANGELOG.md b/CHANGELOG.md index eaa6b11ae8..25ae2d23cd 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/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][]) diff --git a/config/default.yml b/config/default.yml index 88ab9e12d6..766a415ec5 100644 --- a/config/default.yml +++ b/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 diff --git a/docs/modules/ROOT/pages/cops.adoc b/docs/modules/ROOT/pages/cops.adoc index 69915512d3..b10fd09a7e 100644 --- a/docs/modules/ROOT/pages/cops.adoc +++ b/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] diff --git a/docs/modules/ROOT/pages/cops_rails.adoc b/docs/modules/ROOT/pages/cops_rails.adoc index 96095f8eae..3788361902 100644 --- a/docs/modules/ROOT/pages/cops_rails.adoc +++ b/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 |=== diff --git a/lib/rubocop/cop/rails/active_record_find.rb b/lib/rubocop/cop/rails/active_record_find.rb new file mode 100644 index 0000000000..719e768993 --- /dev/null +++ b/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 `%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 a999a8142a..e2c73dfbdf 100644 --- a/lib/rubocop/cop/rails_cops.rb +++ b/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' diff --git a/spec/rubocop/cop/rails/active_record_find_spec.rb b/spec/rubocop/cop/rails/active_record_find_spec.rb new file mode 100644 index 0000000000..727b127225 --- /dev/null +++ b/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