diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 5744055618..75ad458f85 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -1,18 +1,11 @@ # This configuration was generated by # `rubocop --auto-gen-config` -# on 2025-01-22 15:33:45 UTC using RuboCop version 1.70.0. +# on 2025-03-12 19:37:57 UTC using RuboCop version 1.61.0. # The point is for the user to remove these configuration records # one by one as the offenses are removed from the code base. # Note that changes in the inspected code, or installation of new # versions of RuboCop, may require this file to be generated again. -# Offense count: 2 -# This cop supports safe autocorrection (--autocorrect). -InternalAffairs/CopEnabled: - Exclude: - - 'lib/rubocop/cop/rails/blank.rb' - - 'lib/rubocop/cop/rails/present.rb' - # Offense count: 7 InternalAffairs/NodeDestructuring: Exclude: @@ -22,33 +15,36 @@ InternalAffairs/NodeDestructuring: - 'lib/rubocop/cop/rails/relative_date_constant.rb' - 'lib/rubocop/cop/rails/time_zone.rb' -# Offense count: 84 -InternalAffairs/OnSendWithoutOnCSend: - Enabled: false +# Offense count: 2 +# This cop supports safe autocorrection (--autocorrect). +# Configuration parameters: AllowedMethods. +# AllowedMethods: present?, blank?, presence, try, try! +Lint/SafeNavigationConsistency: + Exclude: + - 'lib/rubocop/cop/rails/squished_sql_heredocs.rb' + - 'lib/rubocop/cop/rails/where_range.rb' # Offense count: 10 # Configuration parameters: CountComments, CountAsOne. Metrics/ClassLength: - Max: 163 - Exclude: - - 'lib/rubocop/cop/rails/file_path.rb' + Max: 197 -# Offense count: 41 +# Offense count: 42 # Configuration parameters: CountComments, CountAsOne, AllowedMethods, AllowedPatterns. Metrics/MethodLength: Max: 14 -# Offense count: 183 +# Offense count: 185 # Configuration parameters: Prefixes, AllowedPatterns. # Prefixes: when, with, without RSpec/ContextWording: Enabled: false -# Offense count: 1091 +# Offense count: 1096 # Configuration parameters: CountAsOne. RSpec/ExampleLength: Max: 108 -# Offense count: 733 +# Offense count: 739 RSpec/MultipleExpectations: Max: 5 diff --git a/changelog/new_pluck_on_select_cop.md b/changelog/new_pluck_on_select_cop.md new file mode 100644 index 0000000000..0d795abf10 --- /dev/null +++ b/changelog/new_pluck_on_select_cop.md @@ -0,0 +1 @@ +* [#1435](https://github.com/rubocop/rubocop-rails/issues/1435): Add new `Rails/PluckOnSelect` cop. ([@blnoonan][]) diff --git a/config/default.yml b/config/default.yml index a14fa09acd..6f0d338a0a 100644 --- a/config/default.yml +++ b/config/default.yml @@ -784,6 +784,15 @@ Rails/PluckInWhere: - conservative - aggressive +Rails/PluckOnSelect: + Description: 'Do not use `pluck` on `select`.' + Enabled: 'pending' + VersionAdded: '<>' + EnforcedStyle: conservative + SupportedStyles: + - conservative + - aggressive + Rails/PluralizationGrammar: Description: 'Checks for incorrect grammar when using methods like `3.day.ago`.' Enabled: true diff --git a/lib/rubocop/cop/rails/pluck_on_select.rb b/lib/rubocop/cop/rails/pluck_on_select.rb new file mode 100644 index 0000000000..21aa1484c7 --- /dev/null +++ b/lib/rubocop/cop/rails/pluck_on_select.rb @@ -0,0 +1,135 @@ +# typed: false +# frozen_string_literal: true + +module RuboCop + module Cop + module Rails + # Do not use .pluck on .select. + # + # - .select returns an ActiveRecord relation with only the selected column(s) marked for retrieval + # - .pluck returns an array of column values + # + # Using them together is at best redundant and at worst confusing and inefficient. + # When chained with .select, .pluck is unaware of any directive passed to .select + # (e.g. column aliases or a DISTINCT clause). This can lead to unexpected behavior. + # + # @example + # + # # before + # User.select(:id).pluck(:id) + # + # # after + # User.pluck(:id) + # + # # The .select is redundant. Use either .select or .pluck on its own. + # + # @example + # + # # before + # User.select('id, email AS user_email').pluck('id', 'user_email') + # + # # after + # User.pluck(:id, :email) + # + # # after + # User.select(:id, 'email AS user_email') + # + # # .pluck is unaware of the alias created by .select and will raise an "Unknown column" error. + # # If you need the alias, use .select on its own. Otherwise, consider using .pluck on its own. + # + # @example + # + # # before + # User.select('DISTINCT email').pluck(:email) + # + # # after + # User.group(:email).pluck(:email) + # + # # after + # User.distinct.pluck(:email) + # + # # after + # User.distinct.select(:email) + # + # # .pluck is unaware of .select's DISTINCT directive and will load all User emails from the + # # database - including duplicates. Use either .select or .pluck on its own with .distinct, + # # or use .group (which can be more efficient). + # + # @example + # + # # before + # User.select(:company_id).distinct.pluck(:company_id) + # + # # after + # User.group(:company_id).pluck(:company_id) + # + # # after + # User.distinct.pluck(:company_id) + # + # # after + # User.distinct.select(:company_id) + # + # # The .select is redundant. Use either .select or .pluck on its own with .distinct, + # # or use .group (which can be more efficient). + # + # @example EnforcedStyle: aggressive + # + # # before + # User.select(&:active?).pluck(:id) + # + # # after + # User.where(active: true).pluck(:id) + # + # # after (caution - potentially memory-intensive) + # User.select(&:active?).map(&:id) + # + # # after (caution - potentially memory-intensive) + # User.filter(&:active?).map(&:id) + # + # # .select and .pluck make this statement look like an ActiveRecord operation, but under the hood + # # .select is loading all Users into memory before filtering them using the active? method in Ruby. + # # Use .where to avoid loading all Users into memory, or use .filter or .map to make it + # # clear to readers this is not an ActiveRecord operation. + # + class PluckOnSelect < Base + include ConfigurableEnforcedStyle + + RESTRICT_ON_SEND = %i[pluck].freeze + MSG = 'Do not use `.pluck` on `.select`.' + + def on_send(node) + return unless node.receiver + + # Check if any receiver in the chain is a select method + add_offense(node) if contains_select_in_chain?(node) + end + + private + + # Helper method to check if the chain contains a select method + def contains_select_in_chain?(node) + return false unless node.receiver + + receiver = node.receiver + while receiver + if receiver.send_type? && receiver.method?(:select) + # For conservative style, ignore if select has a block argument + return false if style == :conservative && block_argument?(receiver) + + return true + end + receiver = receiver.receiver + end + false + end + + def block_argument?(node) + # Check if the first argument is a block pass (&:something) + node.first_argument&.block_pass_type? + end + + alias on_csend on_send + end + end + end +end diff --git a/lib/rubocop/cop/rails_cops.rb b/lib/rubocop/cop/rails_cops.rb index d3b24ec9e9..3cfc46db20 100644 --- a/lib/rubocop/cop/rails_cops.rb +++ b/lib/rubocop/cop/rails_cops.rb @@ -89,6 +89,7 @@ require_relative 'rails/pluck' require_relative 'rails/pluck_id' require_relative 'rails/pluck_in_where' +require_relative 'rails/pluck_on_select' require_relative 'rails/pluralization_grammar' require_relative 'rails/presence' require_relative 'rails/present' diff --git a/spec/rubocop/cop/rails/pluck_on_select_spec.rb b/spec/rubocop/cop/rails/pluck_on_select_spec.rb new file mode 100644 index 0000000000..f468c46bc5 --- /dev/null +++ b/spec/rubocop/cop/rails/pluck_on_select_spec.rb @@ -0,0 +1,171 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::Rails::PluckOnSelect, :config do + let(:cop_config) do + { 'EnforcedStyle' => enforced_style } + end + + context 'EnforcedStyle: aggressive' do + let(:enforced_style) { 'aggressive' } + + it 'registers an offense when using pluck on select' do + expect_offense(<<~RUBY) + User.select(:id).pluck(:id) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not use `.pluck` on `.select`. + RUBY + end + + it 'registers an offense when using pluck on select with string ID' do + expect_offense(<<~RUBY) + User.select('id').pluck(:id) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not use `.pluck` on `.select`. + RUBY + end + + it 'registers an offense when using pluck on select with alias' do + expect_offense(<<~RUBY) + User.select('id AS id2').pluck('id2') + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not use `.pluck` on `.select`. + RUBY + end + + it 'registers an offense when using pluck on select with distinct' do + expect_offense(<<~RUBY) + User.select('DISTINCT id').pluck(:id) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not use `.pluck` on `.select`. + RUBY + end + + it 'registers an offense when using pluck on select higher up in the node' do + expect_offense(<<~RUBY) + User.select('id AS id2').distinct.pluck(:id2) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not use `.pluck` on `.select`. + RUBY + end + + it 'registers an offense when using pluck on select with a method filter' do + expect_offense(<<~RUBY) + User.select(&:active?).pluck(:id) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not use `.pluck` on `.select`. + RUBY + end + + it 'registers an offense when using pluck on select with multiple columns' do + expect_offense(<<~RUBY) + User.select(:id, :name).pluck(:id, :name) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not use `.pluck` on `.select`. + RUBY + end + + it 'registers an offense when using pluck on select with intermediate methods' do + expect_offense(<<~RUBY) + User.select(:id).where(active: true).pluck(:id) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not use `.pluck` on `.select`. + RUBY + end + + it 'registers an offense when using pluck on select with complex chaining' do + expect_offense(<<~RUBY) + User.joins(:posts).select('users.id').order(created_at: :desc).pluck('users.id') + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not use `.pluck` on `.select`. + RUBY + end + + it 'does not register an offense when using pluck without select' do + expect_no_offenses(<<~RUBY) + User.pluck(:id) + RUBY + end + + it 'does not register an offense when using pluck without receiver' do + expect_no_offenses(<<~RUBY) + pluck(:id) + RUBY + end + + it 'does not register an offense when using select and pluck in separate chains' do + expect_no_offenses(<<~RUBY) + users = User.select(:id) + ids = User.pluck(:id) + RUBY + end + end + + context 'EnforcedStyle: conservative' do + let(:enforced_style) { 'conservative' } + + it 'registers an offense when using pluck on select' do + expect_offense(<<~RUBY) + User.select(:id).pluck(:id) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not use `.pluck` on `.select`. + RUBY + end + + it 'registers an offense when using pluck on select with string ID' do + expect_offense(<<~RUBY) + User.select('id').pluck(:id) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not use `.pluck` on `.select`. + RUBY + end + + it 'registers an offense when using pluck on select with alias' do + expect_offense(<<~RUBY) + User.select('id AS id2').pluck('id2') + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not use `.pluck` on `.select`. + RUBY + end + + it 'registers an offense when using pluck on select with distinct' do + expect_offense(<<~RUBY) + User.select('DISTINCT id').pluck(:id) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not use `.pluck` on `.select`. + RUBY + end + + it 'registers an offense when using pluck on select higher up in the node' do + expect_offense(<<~RUBY) + User.select('id AS id2').distinct.pluck(:id2) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not use `.pluck` on `.select`. + RUBY + end + + it 'registers an offense when using pluck on select with multiple columns' do + expect_offense(<<~RUBY) + User.select(:id, :name).pluck(:id, :name) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not use `.pluck` on `.select`. + RUBY + end + + it 'registers an offense when using pluck on select with intermediate methods' do + expect_offense(<<~RUBY) + User.select(:id).where(active: true).pluck(:id) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not use `.pluck` on `.select`. + RUBY + end + + it 'does not register an offense when using pluck on select with a method filter' do + expect_no_offenses(<<~RUBY) + User.select(&:active?).pluck(:id) + RUBY + end + + it 'does not register an offense when using pluck without select' do + expect_no_offenses(<<~RUBY) + User.pluck(:id) + RUBY + end + + it 'does not register an offense when using pluck without receiver' do + expect_no_offenses(<<~RUBY) + pluck(:id) + RUBY + end + + it 'does not register an offense when using select and pluck in separate chains' do + expect_no_offenses(<<~RUBY) + users = User.select(:id) + ids = User.pluck(:id) + RUBY + end + end +end