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.

https://github.com/rubocop/rubocop-rails/blob/1c4c37e8d3b8819ca2968cc14fea0606c4624e33/lib/rubocop/cop/rails/strong_parameters_expect.rb#L8-L12

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.

[calebmeyer]

It also fails for multiple requires:

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

is auto-corrected to

params.expect([:foo, :bar] => [:baz]

which doesn't work.

[crespire]

This cop's autocorrect caused quite a lot of pain for us until we realized it was autocorrected and incorrectly.

We had a bunch of custom keys being compiled and those were not being properly handled by Rubocop autocorrect.

We went from:

def permitted_keys
  %i[
    some
    custom
    keys
  ]
end

# elsewhere
params.require(:model).permit(permitted_keys)

to:

params.expect(model: [permitted_keys])

which caused all keys to get filtered causing a lot of errors.

We also have other areas of the app where we use the require.permit pattern for properties that might not be present, and these were converted to expect which is a lot more strict.

[Petercopter]

Bump. Just tried to run autocorrect for params and it's not handling nested arrays of attributes correctly.