diff --git a/CHANGELOG.md b/CHANGELOG.md index 006980c455..be369485fe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ * [#312](https://github.com/rubocop-hq/rubocop-rails/pull/312): Mark `Rails/MailerName` as unsafe for auto-correct. ([@eugeneius][]) * [#294](https://github.com/rubocop-hq/rubocop-rails/pull/294): Update `Rails/ReversibleMigration` to register offenses for `remove_columns` and `remove_index`. ([@philcoggins][]) +* [#310](https://github.com/rubocop-hq/rubocop-rails/issues/310): Add `EnforcedStyle` to `Rails/PluckInWhere`. By default, it does not register an offense if `pluck` method's receiver is a variable. ([@koic][]) ## 2.7.1 (2020-07-26) diff --git a/config/default.yml b/config/default.yml index ff1cb992a1..2da68ebc49 100644 --- a/config/default.yml +++ b/config/default.yml @@ -435,6 +435,11 @@ Rails/PluckInWhere: Enabled: 'pending' Safe: false VersionAdded: '2.7' + VersionChanged: '2.8' + EnforcedStyle: conservative + SupportedStyles: + - conservative + - aggressive Rails/PluralizationGrammar: Description: 'Checks for incorrect grammar when using methods like `3.day.ago`.' diff --git a/docs/modules/ROOT/pages/cops_rails.adoc b/docs/modules/ROOT/pages/cops_rails.adoc index 9290634331..630ead0754 100644 --- a/docs/modules/ROOT/pages/cops_rails.adoc +++ b/docs/modules/ROOT/pages/cops_rails.adoc @@ -2495,7 +2495,7 @@ end | No | Yes (Unsafe) | 2.7 -| - +| 2.8 |=== This cop identifies places where `pluck` is used in `where` query methods @@ -2504,6 +2504,18 @@ and can be replaced with `select`. Since `pluck` is an eager method and hits the database immediately, using `select` helps to avoid additional database queries. +This cop has two different enforcement modes. When the EnforcedStyle +is conservative (the default) then only calls to `pluck` on a constant +(i.e. a model class) in the `where` is used as offenses. + +When the EnforcedStyle is aggressive then all calls to `pluck` in the +`where` is used as offenses. This may lead to false positives +as the cop cannot replace to `select` between calls to `pluck` on an +`ActiveRecord::Relation` instance vs a call to `pluck` on an `Array` instance. + +Autocorrect is disabled by default for this cop since it may generate +false positives. + === Examples [source,ruby] @@ -2513,7 +2525,34 @@ Post.where(user_id: User.active.pluck(:id)) # good Post.where(user_id: User.active.select(:id)) +Post.where(user_id: active_users.select(:id)) +---- + +==== EnforcedStyle: conservative (default) + +[source,ruby] +---- +# good +Post.where(user_id: active_users.pluck(:id)) +---- + +==== EnforcedStyle: aggressive + +[source,ruby] ---- +# bad +Post.where(user_id: active_users.pluck(:id)) +---- + +=== Configurable attributes + +|=== +| Name | Default value | Configurable values + +| EnforcedStyle +| `conservative` +| `conservative`, `aggressive` +|=== == Rails/PluralizationGrammar diff --git a/lib/rubocop/cop/rails/pluck_in_where.rb b/lib/rubocop/cop/rails/pluck_in_where.rb index dfd81181fe..69481a744a 100644 --- a/lib/rubocop/cop/rails/pluck_in_where.rb +++ b/lib/rubocop/cop/rails/pluck_in_where.rb @@ -9,20 +9,45 @@ module Rails # Since `pluck` is an eager method and hits the database immediately, # using `select` helps to avoid additional database queries. # + # This cop has two different enforcement modes. When the EnforcedStyle + # is conservative (the default) then only calls to `pluck` on a constant + # (i.e. a model class) in the `where` is used as offenses. + # + # When the EnforcedStyle is aggressive then all calls to `pluck` in the + # `where` is used as offenses. This may lead to false positives + # as the cop cannot replace to `select` between calls to `pluck` on an + # `ActiveRecord::Relation` instance vs a call to `pluck` on an `Array` instance. + # + # Autocorrect is disabled by default for this cop since it may generate + # false positives. + # # @example # # bad # Post.where(user_id: User.active.pluck(:id)) # # # good # Post.where(user_id: User.active.select(:id)) + # Post.where(user_id: active_users.select(:id)) + # + # @example EnforcedStyle: conservative (default) + # # good + # Post.where(user_id: active_users.pluck(:id)) + # + # @example EnforcedStyle: aggressive + # # bad + # Post.where(user_id: active_users.pluck(:id)) # class PluckInWhere < Cop include ActiveRecordHelper + include ConfigurableEnforcedStyle MSG = 'Use `select` instead of `pluck` within `where` query method.' def on_send(node) - add_offense(node, location: :selector) if node.method?(:pluck) && in_where?(node) + return unless node.method?(:pluck) && in_where?(node) + return if style == :conservative && !root_receiver(node)&.const_type? + + add_offense(node, location: :selector) end def autocorrect(node) @@ -30,6 +55,18 @@ def autocorrect(node) corrector.replace(node.loc.selector, 'select') end end + + private + + def root_receiver(node) + receiver = node.receiver + + if receiver&.send_type? + root_receiver(receiver) + else + receiver + end + end end end end diff --git a/spec/rubocop/cop/rails/pluck_in_where_spec.rb b/spec/rubocop/cop/rails/pluck_in_where_spec.rb index 08bd5162df..a8f10fb257 100644 --- a/spec/rubocop/cop/rails/pluck_in_where_spec.rb +++ b/spec/rubocop/cop/rails/pluck_in_where_spec.rb @@ -1,45 +1,84 @@ # frozen_string_literal: true -RSpec.describe RuboCop::Cop::Rails::PluckInWhere do - subject(:cop) { described_class.new } - - it 'registers an offense and corrects when using `pluck` in `where`' do - expect_offense(<<~RUBY) - Post.where(user_id: User.pluck(:id)) - ^^^^^ Use `select` instead of `pluck` within `where` query method. - RUBY - - expect_correction(<<~RUBY) - Post.where(user_id: User.select(:id)) - RUBY +RSpec.describe RuboCop::Cop::Rails::PluckInWhere, :config do + subject(:cop) { described_class.new(config) } + + let(:cop_config) do + { 'EnforcedStyle' => enforced_style } end - it 'registers an offense and corrects when using `pluck` in `rewhere`' do - expect_offense(<<~RUBY) - Post.rewhere('user_id IN (?)', User.pluck(:id)) - ^^^^^ Use `select` instead of `pluck` within `where` query method. - RUBY + shared_examples 'receiver is a constant for `pluck`' do + it 'registers an offense and corrects when using `pluck` in `where` for constant' do + expect_offense(<<~RUBY) + Post.where(user_id: User.active.pluck(:id)) + ^^^^^ Use `select` instead of `pluck` within `where` query method. + RUBY - expect_correction(<<~RUBY) - Post.rewhere('user_id IN (?)', User.select(:id)) - RUBY - end + expect_correction(<<~RUBY) + Post.where(user_id: User.active.select(:id)) + RUBY + end + + it 'registers an offense and corrects when using `pluck` in `rewhere` for constant' do + expect_offense(<<~RUBY) + Post.rewhere('user_id IN (?)', User.active.pluck(:id)) + ^^^^^ Use `select` instead of `pluck` within `where` query method. + RUBY + + expect_correction(<<~RUBY) + Post.rewhere('user_id IN (?)', User.active.select(:id)) + RUBY + end + + it 'does not register an offense when using `select` in `where`' do + expect_no_offenses(<<~RUBY) + Post.where(user_id: User.active.select(:id)) + RUBY + end + + it 'does not register an offense when using `pluck` chained with other method calls in `where`' do + expect_no_offenses(<<~RUBY) + Post.where(user_id: User.pluck(:id).map(&:to_i)) + RUBY + end - it 'does not register an offense when using `select` in `where`' do - expect_no_offenses(<<~RUBY) - Post.where(user_id: User.select(:id)) - RUBY + it 'does not register an offense when using `select` in query methods other than `where`' do + expect_no_offenses(<<~RUBY) + Post.order(columns.pluck(:name)) + RUBY + end end - it 'does not register an offense when using `pluck` chained with other method calls in `where`' do - expect_no_offenses(<<~RUBY) - Post.where(user_id: User.pluck(:id).map(&:to_i)) - RUBY + context 'EnforcedStyle: conservative' do + let(:enforced_style) { 'conservative' } + + it_behaves_like 'receiver is a constant for `pluck`' + + context 'receiver is a variable for `pluck`' do + it 'does not register an offense when using `pluck` in `where`' do + expect_no_offenses(<<~RUBY) + Post.where(user_id: users.active.pluck(:id)) + RUBY + end + end end - it 'does not register an offense when using `select` in query methods other than `where`' do - expect_no_offenses(<<~RUBY) - Post.order(columns.pluck(:name)) - RUBY + context 'EnforcedStyle: aggressive' do + let(:enforced_style) { 'aggressive' } + + it_behaves_like 'receiver is a constant for `pluck`' + + context 'receiver is a variable for `pluck`' do + it 'registers and corrects an offense when using `pluck` in `where`' do + expect_offense(<<~RUBY) + Post.where(user_id: users.active.pluck(:id)) + ^^^^^ Use `select` instead of `pluck` within `where` query method. + RUBY + + expect_correction(<<~RUBY) + Post.where(user_id: users.active.select(:id)) + RUBY + end + end end end