Merge pull request #1626 from koic/fix_false_positives_for_rails_strong_parameters_expect_with_collection_methods

[Fix #1625] Fix false positives in `Rails/StrongParametersExpect`

Author: koic

changelog/fix_false_positives_for_rails_strong_parameters_expect_with_collection_methods.md

@@ -0,0 +1 @@
+* [#1625](https://github.com/rubocop/rubocop-rails/issues/1625): Fix false positives in `Rails/StrongParametersExpect` when using collection methods (such as `delete`, `keys`, `merge`, `slice`, `dig`, `fetch`, or `transform_values`) on `params[:key]`, as well as block-style calls such as `params[:key].each { ... }` or `params[:key].map(&:to_s)`. ([@koic][])

lib/rubocop/cop/rails/strong_parameters_expect.rb

@@ -9,7 +9,9 @@ module Rails
       # and the cop detects it using the `expect` method.
       #
       # - Method calls on `params[:key]` without comparison methods, methods that are safe to call
-      #   on `nil` (such as `to_i`, `to_s`, or `is_a?`), or key-check methods such as `key?`
+      #   on `nil` (such as `to_i`, `to_s`, or `is_a?`), key-check methods such as `key?`,
+      #   collection methods such as `keys`, `merge`, or `slice`, or block-style calls such as
+      #   `params[:key].each { ... }` or `params[:key].map(&:to_s)`
       # - Passing `params[:key]` as an argument to finder methods that raise on missing records
       # - Strong parameter methods using `require` or `permit`
       #
@@ -54,9 +56,18 @@ class StrongParametersExpect < Base
 
         MSG = 'Use `%<prefer>s` instead.'
         RESTRICT_ON_SEND = %i[[] require permit].freeze
-        PRESENCE_CHECK_METHODS = %i[nil? blank? present? presence].freeze
-        NIL_SAFE_METHODS = %i[instance_of? is_a? kind_of? to_a to_f to_h to_i to_s try try!].freeze
-        KEY_CHECK_METHODS = %i[key? has_key? include? member?].freeze
+        # Method calls on `params[:key]` that should not be rewritten with `expect(:key)`.
+        # Covers presence/nil checks, nil-safe conversions and type checks, key-check methods,
+        # and collection methods that imply `params[:key]` is a Hash/Array.
+        IGNORED_METHODS = %i[
+          blank? compact compact! compact_blank compact_blank! deep_merge deep_merge!
+          delete delete_if dig each except exclude? extract! fetch has_key? has_value?
+          include? instance_of? is_a? keep_if key? keys kind_of? member? merge merge!
+          nil? presence present? reverse_merge reverse_merge! slice stringify_keys
+          to_a to_f to_h to_hash to_i to_s to_unsafe_h to_unsafe_hash
+          transform_keys transform_keys! transform_values transform_values! try try!
+          value? values values_at with_defaults with_defaults! without
+        ].freeze
         RAISING_FINDER_METHODS = %i[find find_by! find_sole_by].freeze
 
         minimum_target_rails_version 8.0
@@ -135,12 +146,9 @@ def offensive_bracket_access?(node)
 
           if parent.receiver == node
             return false if parent.comparison_method? || parent.method?(:[])
+            return false if block_call?(parent)
 
-            method_name = parent.method_name
-
-            !PRESENCE_CHECK_METHODS.include?(method_name) &&
-              !NIL_SAFE_METHODS.include?(method_name) &&
-              !KEY_CHECK_METHODS.include?(method_name)
+            !IGNORED_METHODS.include?(parent.method_name)
           else
             raising_finder_method?(parent)
           end
@@ -151,6 +159,10 @@ def raising_finder_method?(node)
           RAISING_FINDER_METHODS.include?(node.method_name)
         end
 
+        def block_call?(send_node)
+          send_node.block_literal? || send_node.last_argument&.block_pass_type?
+        end
+
         def offense_range(method_node, node)
           method_node.loc.selector.join(node.source_range.end)
         end

spec/rubocop/cop/rails/strong_parameters_expect_spec.rb

@@ -109,6 +109,98 @@
       RUBY
     end
 
+    it 'does not register an offense when using `params[:key].delete(:inner)`' do
+      expect_no_offenses(<<~RUBY)
+        params[:key].delete(:inner)
+      RUBY
+    end
+
+    it 'does not register an offense when using `params[:key].each do |_index, subparams|`' do
+      expect_no_offenses(<<~RUBY)
+        params[:key].each do |_index, subparams|
+          do_something(subparams)
+        end
+      RUBY
+    end
+
+    it 'does not register an offense when using `params[:key].map { ... }`' do
+      expect_no_offenses(<<~RUBY)
+        params[:key].map { |item| item[:id] }
+      RUBY
+    end
+
+    it 'does not register an offense when using `params[:key].select(&:present?)`' do
+      expect_no_offenses(<<~RUBY)
+        params[:key].select(&:present?)
+      RUBY
+    end
+
+    it 'does not register an offense when using `params[:key].reject { |k, v| v.blank? }`' do
+      expect_no_offenses(<<~RUBY)
+        params[:key].reject { |_k, v| v.blank? }
+      RUBY
+    end
+
+    it 'does not register an offense when using `params[:key].keys`' do
+      expect_no_offenses(<<~RUBY)
+        params[:key].keys
+      RUBY
+    end
+
+    it 'does not register an offense when using `params[:key].values`' do
+      expect_no_offenses(<<~RUBY)
+        params[:key].values
+      RUBY
+    end
+
+    it 'does not register an offense when using `params[:key].slice(:a, :b)`' do
+      expect_no_offenses(<<~RUBY)
+        params[:key].slice(:a, :b)
+      RUBY
+    end
+
+    it 'does not register an offense when using `params[:key].except(:a)`' do
+      expect_no_offenses(<<~RUBY)
+        params[:key].except(:a)
+      RUBY
+    end
+
+    it 'does not register an offense when using `params[:key].merge(other)`' do
+      expect_no_offenses(<<~RUBY)
+        params[:key].merge(other)
+      RUBY
+    end
+
+    it 'does not register an offense when using `params[:key].dig(:a, :b)`' do
+      expect_no_offenses(<<~RUBY)
+        params[:key].dig(:a, :b)
+      RUBY
+    end
+
+    it 'does not register an offense when using `params[:key].fetch(:inner)`' do
+      expect_no_offenses(<<~RUBY)
+        params[:key].fetch(:inner)
+      RUBY
+    end
+
+    it 'does not register an offense when using `params[:key].compact`' do
+      expect_no_offenses(<<~RUBY)
+        params[:key].compact
+      RUBY
+    end
+
+    it 'does not register an offense when using `params[:key].transform_values(&:to_s)`' do
+      expect_no_offenses(<<~RUBY)
+        params[:key].transform_values(&:to_s)
+      RUBY
+    end
+
+    it 'does not register an offense when using `params[:key].to_unsafe_h`' do
+      expect_no_offenses(<<~RUBY)
+        params[:key].to_unsafe_h
+      RUBY
+    end
+
     it "does not register an offense when using `params[:key] == 'value'`" do
       expect_no_offenses(<<~RUBY)
         params[:key] == 'value'