Rails/StrongParametersExpect can break working code - this should be documented

[JasonBarnabe]

The current documentation for Rails/StrongParametersExpect says that it's unsafe because the response code on invalid parameters changes from 500 to 400.

rubocop-rails/lib/rubocop/cop/rails/strong_parameters_expect.rb

Lines 8 to 12 in 1c4c37e

	# @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.

But it's also unsafe because ActionController::Parameters#expect is pickier about the format and may change some successful requests into failures.

https://api.rubyonrails.org/classes/ActionController/Parameters.html#method-i-expect

This should be documented in the cop.

Example

The cop turned this code:

   def discussion_params
     attrs = [:rating, :title, :discussion_category_id, :report_id, { comments_attributes: [:text, :text_markup, { attachments: [] }] }]
     attrs += [:report_id] if current_user&.moderator?
     params
       .require(:discussion)
       .permit(attrs)
   end

into this (after correction for #1417):

   def discussion_params
     attrs = [:rating, :title, :discussion_category_id, :report_id, { comments_attributes: [:text, :text_markup, { attachments: [] }] }]
     attrs += [:report_id] if current_user&.moderator?
     params.expect(discussion: [attrs])
   end

which now fails on these request parameters:

{"authenticity_token" => "[FILTERED]", "discussion" => {"comments_attributes" => {"0" => {"text_markup" => "html", "text" => "this is my comment", "attachments" => [""]}}, "rating" => "4"}, "subscribe" => "1", "locale" => "en", "script_id" => "1-a-test"

The problem seems to be the lack of double array brackets on the value for comments_attributes. Corrected code:

   def discussion_params
     attrs = [:rating, :title, :discussion_category_id, :report_id, { comments_attributes: [[:text, :text_markup, { attachments: [] }]] }]
     attrs += [:report_id] if current_user&.moderator?
     params.expect(discussion: [attrs])
   end

(The extra array also works with #require/#permit.)

Rubocop version

1.70.0 (using Parser 3.3.7.0, rubocop-ast 1.37.0, analyzing as Ruby 3.4, running on ruby 3.4.1) +server [x86_64-linux]
  - rubocop-capybara 2.21.0
  - rubocop-minitest 0.36.0
  - rubocop-performance 1.23.1
  - rubocop-rails 2.29.0

[lucaong-gst]

As a matter of fact, I do not think this check should be auto-corrected. A manual decision is always necessary in this common case:

params.require(:foo).permit(bar: [:baz])

If the key :bar contains a single object/hash, Rubocop is correctly changing this to:

params.expect(foo: { bar: [:baz] })

But if :bar contains an array, it should be changed to this instead:

params.expect(foo: { bar: [[:baz]] })

I don't think that Rubocop can tell automatically which one is the case.

It is also quite common for issues introduced by this auto fix to silently fail, and even pass specs if the specs do not explicitly make use of the inner array.