Merge pull request #1412 from koic/add_new_rails_strong_parameters_ex…

…pect_cop

Add new `Rails/StrongParametersExpect` cop

changelog/new_add_new_rails_strong_parameters_expect_cop.md

@@ -0,0 +1 @@
+* [#1358](https://github.com/rubocop/rubocop-rails/issues/1358): Add new `Rails/StrongParametersExpect` cop. ([@koic][])

config/default.yml

@@ -1081,6 +1081,15 @@ Rails/StripHeredoc:
   Enabled: pending
   VersionAdded: '2.15'
 
+Rails/StrongParametersExpect:
+  Description: 'Enforces the use of `ActionController::Parameters#expect` as a method for strong parameter handling.'
+  Reference: 'https://api.rubyonrails.org/classes/ActionController/Parameters.html#method-i-expect'
+  Enabled: pending
+  Include:
+    - app/controllers/**/*.rb
+  SafeAutoCorrect: false
+  VersionAdded: '<<next>>'
+
 Rails/TableNameAssignment:
   Description: >-
     Do not use `self.table_name =`. Use Inflections or `table_name_prefix` instead.

lib/rubocop/cop/rails/strong_parameters_expect.rb

@@ -0,0 +1,104 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module Rails
+      # Enforces the use of `ActionController::Parameters#expect` as a method for strong parameter handling.
+      #
+      # @safety
+      #   This cop's autocorrection is considered unsafe because there are cases where the HTTP status may change
+      #   from 500 to 400 when handling invalid parameters. This change, however, reflects an intentional
+      #   incompatibility introduced for valid reasons by the `expect` method, which aligns better with
+      #   strong parameter conventions.
+      #
+      # @example
+      #
+      #   # bad
+      #   params.require(:user).permit(:name, :age)
+      #   params.permit(user: [:name, :age]).require(:user)
+      #
+      #   # good
+      #   params.expect(user: [:name, :age])
+      #
+      class StrongParametersExpect < Base
+        extend AutoCorrector
+        extend TargetRailsVersion
+
+        MSG = 'Use `%<prefer>s` instead.'
+        RESTRICT_ON_SEND = %i[require permit].freeze
+
+        minimum_target_rails_version 8.0
+
+        def_node_matcher :params_require_permit, <<~PATTERN
+          $(call
+            $(call
+              (send nil? :params) :require _) :permit ...)
+        PATTERN
+
+        def_node_matcher :params_permit_require, <<~PATTERN
+          $(call
+            $(call
+               (send nil? :params) :permit (hash (pair _require_param_name _ )))
+                 :require  _require_param_name)
+        PATTERN
+
+        # rubocop:disable Metrics/AbcSize, Metrics/MethodLength
+        def on_send(node)
+          return if part_of_ignored_node?(node)
+
+          if (permit_method, require_method = params_require_permit(node))
+            range = offense_range(require_method, node)
+            prefer = expect_method(require_method, permit_method)
+            replace_argument = true
+          elsif (require_method, permit_method = params_permit_require(node))
+            range = offense_range(permit_method, node)
+            prefer = "expect(#{permit_method.arguments.map(&:source).join(', ')})"
+            replace_argument = false
+          else
+            return
+          end
+
+          add_offense(range, message: format(MSG, prefer: prefer)) do |corrector|
+            corrector.remove(require_method.loc.dot.join(require_method.source_range.end))
+            corrector.replace(permit_method.loc.selector, 'expect')
+            if replace_argument
+              corrector.insert_before(permit_method.first_argument, "#{require_key(require_method)}[")
+              corrector.insert_after(permit_method.last_argument, ']')
+            end
+          end
+
+          ignore_node(node)
+        end
+        # rubocop:enable Metrics/AbcSize, Metrics/MethodLength
+        alias on_csend on_send
+
+        private
+
+        def offense_range(method_node, node)
+          method_node.loc.selector.join(node.source_range.end)
+        end
+
+        def expect_method(require_method, permit_method)
+          require_key = require_key(require_method)
+          permit_args = permit_method.arguments.map(&:source).join(', ')
+
+          arguments = "#{require_key}[#{permit_args}]"
+
+          "expect(#{arguments})"
+        end
+
+        def require_key(require_method)
+          if (first_argument = require_method.first_argument).respond_to?(:value)
+            require_arg = first_argument.value
+            separator = ': '
+          else
+            require_arg = first_argument.source
+            separator = ' => '
+          end
+
+          "#{require_arg}#{separator}"
+        end
+      end
+    end
+  end
+end

lib/rubocop/cop/rails_cops.rb

@@ -11,6 +11,7 @@
 require_relative 'mixin/target_rails_version'
 
 require_relative 'rails/action_controller_flash_before_render'
+require_relative 'rails/strong_parameters_expect'
 require_relative 'rails/action_controller_test_case'
 require_relative 'rails/action_filter'
 require_relative 'rails/action_order'

spec/rubocop/cop/rails/strong_parameters_expect_spec.rb

@@ -0,0 +1,151 @@
+# frozen_string_literal: true
+
+RSpec.describe RuboCop::Cop::Rails::StrongParametersExpect, :config do
+  context 'Rails >= 8.0', :rails80 do
+    it 'registers an offense when using `params.require(:user).permit(:name, :age)`' do
+      expect_offense(<<~RUBY)
+        params.require(:user).permit(:name, :age)
+               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `expect(user: [:name, :age])` instead.
+      RUBY
+
+      expect_correction(<<~RUBY)
+        params.expect(user: [:name, :age])
+      RUBY
+    end
+
+    it 'registers an offense when using `params&.require(:user)&.permit(:name, :age)`' do
+      expect_offense(<<~RUBY)
+        params&.require(:user)&.permit(:name, :age)
+                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `expect(user: [:name, :age])` instead.
+      RUBY
+
+      expect_correction(<<~RUBY)
+        params&.expect(user: [:name, :age])
+      RUBY
+    end
+
+    it 'registers an offense when using `params.permit(user: [:name, :age]).require(:user)`' do
+      expect_offense(<<~RUBY)
+        params.permit(user: [:name, :age]).require(:user)
+               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `expect(user: [:name, :age])` instead.
+      RUBY
+
+      expect_correction(<<~RUBY)
+        params.expect(user: [:name, :age])
+      RUBY
+    end
+
+    it 'registers an offense when using `params&.permit(user: [:name, :age])&.require(:user)`' do
+      expect_offense(<<~RUBY)
+        params&.permit(user: [:name, :age])&.require(:user)
+                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `expect(user: [:name, :age])` instead.
+      RUBY
+
+      expect_correction(<<~RUBY)
+        params&.expect(user: [:name, :age])
+      RUBY
+    end
+
+    it 'registers an offense when using `params.require(:user).permit(:name, some_ids: [])`' do
+      expect_offense(<<~RUBY)
+        params.require(:user).permit(:name, some_ids: [])
+               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `expect(user: [:name, some_ids: []])` instead.
+      RUBY
+
+      expect_correction(<<~RUBY)
+        params.expect(user: [:name, some_ids: []])
+      RUBY
+    end
+
+    it 'registers an offense when using `params.require(:user).permit(*parameters, some_ids: [])`' do
+      expect_offense(<<~RUBY)
+        params.require(:user).permit(*parameters, some_ids: [])
+               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `expect(user: [*parameters, some_ids: []])` instead.
+      RUBY
+
+      expect_correction(<<~RUBY)
+        params.expect(user: [*parameters, some_ids: []])
+      RUBY
+    end
+
+    it 'registers an offense when using `params.require(var).permit(:name, some_ids: [])`' do
+      expect_offense(<<~RUBY)
+        var = :user
+        params.require(var).permit(:name, some_ids: [])
+               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `expect(var => [:name, some_ids: []])` instead.
+      RUBY
+
+      expect_correction(<<~RUBY)
+        var = :user
+        params.expect(var => [:name, some_ids: []])
+      RUBY
+    end
+
+    it "registers an offense when using `params.require(:user).permit(:name, :age)` and `permit`'s args has comment" do
+      expect_offense(<<~RUBY)
+        params.require(:user).permit(
+               ^^^^^^^^^^^^^^^^^^^^^^ Use `expect(user: [:name, :age])` instead.
+          :name, # comment
+          :age # comment
+        )
+      RUBY
+
+      expect_correction(<<~RUBY)
+        params.expect(
+          user: [:name, # comment
+          :age] # comment
+        )
+      RUBY
+    end
+
+    it 'does not register an offense when using `params.expect(user: [:name, :age])`' do
+      expect_no_offenses(<<~RUBY)
+        params.expect(user: [:name, :age])
+      RUBY
+    end
+
+    it 'does not register an offense when using `params.permit(unmatch_require_param: [:name, :age]).require(:user)`' do
+      expect_no_offenses(<<~RUBY)
+        params.permit(unmatch_require_param: [:name, :age]).require(:user)
+      RUBY
+    end
+
+    it 'does not register an offense when using `params.require(:name)`' do
+      expect_no_offenses(<<~RUBY)
+        params.require(:name)
+      RUBY
+    end
+
+    it 'does not register an offense when using `params.permit(:name)`' do
+      expect_no_offenses(<<~RUBY)
+        params.permit(:name)
+      RUBY
+    end
+
+    it 'does not register an offense when using `params[:name]`' do
+      expect_no_offenses(<<~RUBY)
+        params[:name]
+      RUBY
+    end
+
+    it 'does not register an offense when using `params.fetch(:name)`' do
+      expect_no_offenses(<<~RUBY)
+        params.fetch(:name)
+      RUBY
+    end
+
+    it 'does not register an offense when using `params[:user][:name]`' do
+      expect_no_offenses(<<~RUBY)
+        params[:user][:name]
+      RUBY
+    end
+  end
+
+  context 'Rails <= 7.2', :rails72 do
+    it 'does not register an offense when using `params.require(:user).permit(:name, :age)`' do
+      expect_no_offenses(<<~RUBY)
+        params.require(:user).permit(:name, :age)
+      RUBY
+    end
+  end
+end