From 077cac5948ea1d6fd74975df398a408bf4032866 Mon Sep 17 00:00:00 2001 From: masato-bkn Date: Sat, 21 Oct 2023 16:06:45 +0900 Subject: [PATCH] Change `Rails/RedundantActiveRecordAllMethod` to ignore `delete_all` and `destroy_all` when receiver is an association --- ...ails_redundant_active_record_all_method.md | 1 + .../redundant_active_record_all_method.rb | 15 ++++++++- ...redundant_active_record_all_method_spec.rb | 32 +++++++++++++++++++ 3 files changed, 47 insertions(+), 1 deletion(-) create mode 100644 changelog/change_rails_redundant_active_record_all_method.md diff --git a/changelog/change_rails_redundant_active_record_all_method.md b/changelog/change_rails_redundant_active_record_all_method.md new file mode 100644 index 0000000000..9740a0e701 --- /dev/null +++ b/changelog/change_rails_redundant_active_record_all_method.md @@ -0,0 +1 @@ +* [#1171](https://github.com/rubocop/rubocop-rails/pull/1171): Change `Rails/RedundantActiveRecordAllMethod` to ignore `delete_all` and `destroy_all` when receiver is an association. ([@masato-bkn][]) diff --git a/lib/rubocop/cop/rails/redundant_active_record_all_method.rb b/lib/rubocop/cop/rails/redundant_active_record_all_method.rb index 0684cbe389..de6cdcd3b9 100644 --- a/lib/rubocop/cop/rails/redundant_active_record_all_method.rb +++ b/lib/rubocop/cop/rails/redundant_active_record_all_method.rb @@ -5,6 +5,13 @@ module Cop module Rails # Detect redundant `all` used as a receiver for Active Record query methods. # + # NOTE: For the methods `delete_all` and `destroy_all`, + # this cop will only check cases where the receiver is a model. + # It will ignore cases where the receiver is an association (e.g., `user.articles.all.delete_all`). + # This is because omitting `all` from an association changes the methods + # from `ActiveRecord::Relation` to `ActiveRecord::Associations::CollectionProxy`, + # which can affect their behavior. + # # @safety # This cop is unsafe for autocorrection if the receiver for `all` is not an Active Record object. # @@ -144,13 +151,15 @@ class RedundantActiveRecordAllMethod < Base ].to_set.freeze POSSIBLE_ENUMERABLE_BLOCK_METHODS = %i[any? count find none? one? select sum].freeze + SENSITIVE_METHODS_ON_ASSOCIATION = %i[delete_all destroy_all].freeze def_node_matcher :followed_by_query_method?, <<~PATTERN (send (send _ :all) QUERYING_METHODS ...) PATTERN def on_send(node) - return if !followed_by_query_method?(node.parent) || possible_enumerable_block_method?(node) + return unless followed_by_query_method?(node.parent) + return if possible_enumerable_block_method?(node) || sensitive_association_method?(node) return if node.receiver ? allowed_receiver?(node.receiver) : !inherit_active_record_base?(node) range_of_all_method = offense_range(node) @@ -169,6 +178,10 @@ def possible_enumerable_block_method?(node) parent.parent&.block_type? || parent.parent&.numblock_type? || parent.first_argument&.block_pass_type? end + def sensitive_association_method?(node) + !node.receiver&.const_type? && SENSITIVE_METHODS_ON_ASSOCIATION.include?(node.parent.method_name) + end + def offense_range(node) range_between(node.loc.selector.begin_pos, node.source_range.end_pos) end diff --git a/spec/rubocop/cop/rails/redundant_active_record_all_method_spec.rb b/spec/rubocop/cop/rails/redundant_active_record_all_method_spec.rb index 56611a8541..ea1fda8e93 100644 --- a/spec/rubocop/cop/rails/redundant_active_record_all_method_spec.rb +++ b/spec/rubocop/cop/rails/redundant_active_record_all_method_spec.rb @@ -421,6 +421,38 @@ class User < ::ActiveRecord::Base end end + describe "#{described_class}::SENSITIVE_METHODS_ON_ASSOCIATION" do + described_class::SENSITIVE_METHODS_ON_ASSOCIATION.each do |method| + context "using `#{method}`" do + it 'registers an offense when the receiver for `all` is a model' do + expect_offense(<<~RUBY) + User.all.#{method} + ^^^ Redundant `all` detected. + RUBY + end + + it 'does not register an offense when the receiver for `all` is an association' do + expect_no_offenses(<<~RUBY) + user.articles.all.#{method} + RUBY + end + + it 'does not register an offense when the receiver for `all` is a relation' do + expect_no_offenses(<<~RUBY) + users = User.all + users.all.#{method} + RUBY + end + + it 'does not register an offense when `all` has no receiver' do + expect_no_offenses(<<~RUBY) + all.#{method} + RUBY + end + end + end + end + context 'with `AllowedReceivers` config' do let(:cop_config) do { 'AllowedReceivers' => %w[ActionMailer::Preview ActiveSupport::TimeZone] }