Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cop idea: Disallow params.require.permit and params.require in favor of params.expect for rails 8.0 #1358

Closed
Earlopain opened this issue Sep 8, 2024 · 1 comment · Fixed by #1412
Labels
feature request Request for new functionality

Comments

@Earlopain
Copy link
Contributor

Is your feature request related to a problem? Please describe.

Rails added params.expect in rails/rails#51674. There are some problems with require that are nicely explained in that PR (and linked issues) but it basically boils down to this:

params.require(:user).permit(:name) raises if params looks like user=123 (not the expected shape). The newly added expect avoids this problem by making the shape part of the contract.

Describe the solution you'd like

Method docs: https://edgeapi.rubyonrails.org/classes/ActionController/Parameters.html#method-i-expect

params.require(:user).permit(:name)
# => 
params.expect(user: %i[name])

params.require(:user).permit(*permitted_params, some_ids: [])
# => 
params.permit(user: [*permitted_params, [:some_ids]])

params.permit(:name)
# =>
params.expect(:name)

These two method calls will likely be close together, so I believe it would be best for the cop to simply catch these simple cases first.

Additional context

If plain params.require is added as an offense, it must be unsafe. Consider the following case:

# Allows an array/hash
User.find(params.require(:id))
# This does not
User.find(params.expect([:id]))
# If arrays are expected:
User.find(params.expect([[:id]])
# expect can't allow both an array and plain type

There is no replacement for the following (yet?):

params.fetch(:optional, {}).permit(:some_arg)
@koic koic added the feature request Request for new functionality label Nov 20, 2024
@koic
Copy link
Member

koic commented Jan 15, 2025

The following description appears to have a mistake.

params.require(:user).permit(*permitted_params, some_ids: [])
# => 
params.permit(user: [*permitted_params, [:some_ids]])

Here is an example of the parameters in Rails 8.0.1:

params = ActionController::Parameters.new(user: { name: 'Martin', foo: 'abc', bar: 'xyz', some_ids: [42, 43] })
permitted_params = %i[foo bar]

some_ids is missing when [:some_ids]:

params.expect(user: [*permitted_params, [:some_ids]])
=> #<ActionController::Parameters {"foo"=>"abc", "bar"=>"xyz"} permitted: true>

The same as with require(:user).permit..., some_ids should be present when some_ids: []:

params.expect(user: [*permitted_params, some_ids: []])
=> #<ActionController::Parameters {"foo"=>"abc", "bar"=>"xyz", "some_ids"=>[42, 43]} permitted: true>
params.require(:user).permit(*permitted_params, some_ids: [])
=> #<ActionController::Parameters {"foo"=>"abc", "bar"=>"xyz", "some_ids"=>[42, 43]} permitted: true>

Therefore, the bad / good example in this case would be as follows:

# bad
params.require(:user).permit(*permitted_params, some_ids: [])

# good
params.expect(user: [*permitted_params, some_ids: []])

koic added a commit to koic/rubocop-rails that referenced this issue Jan 17, 2025
## Summary

This PR adds new `Rails/ActionControllerParametersExpect` cop that enforces
the use of `ActionController::Parameters#expect` as a method for strong parameter handling.

As a starting point for this Cop, the implementation in this PR will detect the following cases.

```ruby
# bad
params.require(:user).permit(:name, :age)
params.permit(user: [:name, :age]).require(:user)

# good
params.expect(user: [:name, :age])
```

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

## Additional Information

This cop does not detect the following cases for the reasons outlined below.
Consideration will be given to whether these should be provided as separate options.

## `params.permit`

Incompatibilities occur with the returned object.

```ruby
params = ActionController::Parameters.new(id: 42)
# => #<ActionController::Parameters {"id"=>42} permitted: false>
params.permit(:id)
# => #<ActionController::Parameters {"id"=>42} permitted: true>
params.expect(:id)
# => 42
```

## `params.require`

It cannot be determined whether `expect(:ids)` or `expect(ids: [])` should be used for the parameter.

`ids` is `42`:

```ruby
params = ActionController::Parameters.new(ids: 42)
# => #<ActionController::Parameters {"ids"=>42} permitted: false>
params.require(:ids)
# => 42
params.expect(:ids)
# => 42
params.expect(ids: [])
# => param is missing or the value is empty or invalid: ids (ActionController::ParameterMissing)
```

`ids` is `[42, 43]`:

```ruby
params = ActionController::Parameters.new(ids: [42, 43])
# => #<ActionController::Parameters {"ids"=>[42, 43]} permitted: false>
params.require(:ids)
# => [42, 43]
params.expect(:ids)
# => param is missing or the value is empty or invalid: ids (ActionController::ParameterMissing)
params.expect(ids: [])
# => [42, 43]
```

## `params[]` and `params.fetch`

Incompatibilities occur when the value is an array.

```ruby
params = ActionController::Parameters.new(ids: [42, 43])
# => #<ActionController::Parameters {"ids"=>[42, 43]} permitted: false>
params[:ids]
# => [42, 43]
params.fetch(:ids)
# => [42, 43]
params.expect(:ids)
# => param is missing or the value is empty or invalid: ids (ActionController::ParameterMissing)
```

These may be designed and provided separately in the future.

Closes rubocop#1358.
koic added a commit to koic/rubocop-rails that referenced this issue Jan 17, 2025
## Summary

This PR adds new `Rails/ActionControllerParametersExpect` cop that enforces
the use of `ActionController::Parameters#expect` as a method for strong parameter handling.

As a starting point for this Cop, the implementation in this PR will detect the following cases.

```ruby
# bad
params.require(:user).permit(:name, :age)
params.permit(user: [:name, :age]).require(:user)

# good
params.expect(user: [:name, :age])
```

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

## Additional Information

This cop does not detect the following cases for the reasons outlined below.
Consideration will be given to whether these should be provided as separate options.

### `params.permit`

Incompatibilities occur with the returned object.

```ruby
params = ActionController::Parameters.new(id: 42)
# => #<ActionController::Parameters {"id"=>42} permitted: false>
params.permit(:id)
# => #<ActionController::Parameters {"id"=>42} permitted: true>
params.expect(:id)
# => 42
```

### `params.require`

It cannot be determined whether `expect(:ids)` or `expect(ids: [])` should be used for the parameter.

`ids` is `42`:

```ruby
params = ActionController::Parameters.new(ids: 42)
# => #<ActionController::Parameters {"ids"=>42} permitted: false>
params.require(:ids)
# => 42
params.expect(:ids)
# => 42
params.expect(ids: [])
# => param is missing or the value is empty or invalid: ids (ActionController::ParameterMissing)
```

`ids` is `[42, 43]`:

```ruby
params = ActionController::Parameters.new(ids: [42, 43])
# => #<ActionController::Parameters {"ids"=>[42, 43]} permitted: false>
params.require(:ids)
# => [42, 43]
params.expect(:ids)
# => param is missing or the value is empty or invalid: ids (ActionController::ParameterMissing)
params.expect(ids: [])
# => [42, 43]
```

### `params[]` and `params.fetch`

Incompatibilities occur when the value is an array.

```ruby
params = ActionController::Parameters.new(ids: [42, 43])
# => #<ActionController::Parameters {"ids"=>[42, 43]} permitted: false>
params[:ids]
# => [42, 43]
params.fetch(:ids)
# => [42, 43]
params.expect(:ids)
# => param is missing or the value is empty or invalid: ids (ActionController::ParameterMissing)
```

These may be designed and provided separately in the future.

Closes rubocop#1358.
koic added a commit to koic/rubocop-rails that referenced this issue Jan 17, 2025
## Summary

This PR adds new `Rails/ActionControllerParametersExpect` cop that enforces
the use of `ActionController::Parameters#expect` as a method for strong parameter handling.

As a starting point for this Cop, the implementation in this PR will detect the following cases.

```ruby
# bad
params.require(:user).permit(:name, :age)
params.permit(user: [:name, :age]).require(:user)

# good
params.expect(user: [:name, :age])
```

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

## Additional Information

This cop does not detect the following cases for the reasons outlined below.
Consideration will be given to whether these should be provided as separate options.

### `params.permit`

Incompatibilities occur with the returned object.

```ruby
params = ActionController::Parameters.new(id: 42)
# => #<ActionController::Parameters {"id"=>42} permitted: false>
params.permit(:id)
# => #<ActionController::Parameters {"id"=>42} permitted: true>
params.expect(:id)
# => 42
```

### `params.require`

It cannot be determined whether `expect(:ids)` or `expect(ids: [])` should be used for the parameter.

`ids` is `42`:

```ruby
params = ActionController::Parameters.new(ids: 42)
# => #<ActionController::Parameters {"ids"=>42} permitted: false>
params.require(:ids)
# => 42
params.expect(:ids)
# => 42
params.expect(ids: [])
# => param is missing or the value is empty or invalid: ids (ActionController::ParameterMissing)
```

`ids` is `[42, 43]`:

```ruby
params = ActionController::Parameters.new(ids: [42, 43])
# => #<ActionController::Parameters {"ids"=>[42, 43]} permitted: false>
params.require(:ids)
# => [42, 43]
params.expect(:ids)
# => param is missing or the value is empty or invalid: ids (ActionController::ParameterMissing)
params.expect(ids: [])
# => [42, 43]
```

### `params[]` and `params.fetch`

Incompatibilities occur when the value is an array.

```ruby
params = ActionController::Parameters.new(ids: [42, 43])
# => #<ActionController::Parameters {"ids"=>[42, 43]} permitted: false>
params[:ids]
# => [42, 43]
params.fetch(:ids)
# => [42, 43]
params.expect(:ids)
# => param is missing or the value is empty or invalid: ids (ActionController::ParameterMissing)
```

These may be designed and provided separately in the future.

Closes rubocop#1358.
koic added a commit to koic/rubocop-rails that referenced this issue Jan 17, 2025
## Summary

This PR adds new `Rails/StrongParametersExpect` cop that enforces
the use of `ActionController::Parameters#expect` as a method for strong parameter handling.

As a starting point for this Cop, the implementation in this PR will detect the following cases.

```ruby
# bad
params.require(:user).permit(:name, :age)
params.permit(user: [:name, :age]).require(:user)

# good
params.expect(user: [:name, :age])
```

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

## Additional Information

This cop does not detect the following cases for the reasons outlined below.
Consideration will be given to whether these should be provided as separate options.

### `params.permit`

Incompatibilities occur with the returned object.

```ruby
params = ActionController::Parameters.new(id: 42)
# => #<ActionController::Parameters {"id"=>42} permitted: false>
params.permit(:id)
# => #<ActionController::Parameters {"id"=>42} permitted: true>
params.expect(:id)
# => 42
```

### `params.require`

It cannot be determined whether `expect(:ids)` or `expect(ids: [])` should be used for the parameter.

`ids` is `42`:

```ruby
params = ActionController::Parameters.new(ids: 42)
# => #<ActionController::Parameters {"ids"=>42} permitted: false>
params.require(:ids)
# => 42
params.expect(:ids)
# => 42
params.expect(ids: [])
# => param is missing or the value is empty or invalid: ids (ActionController::ParameterMissing)
```

`ids` is `[42, 43]`:

```ruby
params = ActionController::Parameters.new(ids: [42, 43])
# => #<ActionController::Parameters {"ids"=>[42, 43]} permitted: false>
params.require(:ids)
# => [42, 43]
params.expect(:ids)
# => param is missing or the value is empty or invalid: ids (ActionController::ParameterMissing)
params.expect(ids: [])
# => [42, 43]
```

### `params[]` and `params.fetch`

Incompatibilities occur when the value is an array.

```ruby
params = ActionController::Parameters.new(ids: [42, 43])
# => #<ActionController::Parameters {"ids"=>[42, 43]} permitted: false>
params[:ids]
# => [42, 43]
params.fetch(:ids)
# => [42, 43]
params.expect(:ids)
# => param is missing or the value is empty or invalid: ids (ActionController::ParameterMissing)
```

These may be designed and provided separately in the future.

Closes rubocop#1358.
koic added a commit to koic/rubocop-rails that referenced this issue Jan 17, 2025
## Summary

This PR adds new `Rails/StrongParametersExpect` cop that enforces
the use of `ActionController::Parameters#expect` as a method for strong parameter handling.

As a starting point for this cop, the implementation in this PR will detect the following cases.

```ruby
# bad
params.require(:user).permit(:name, :age)
params.permit(user: [:name, :age]).require(:user)

# good
params.expect(user: [:name, :age])
```

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

## Additional Information

This cop does not detect the following cases for the reasons outlined below.
Consideration will be given to whether these should be provided as separate options.

### `params.permit`

Incompatibilities occur with the returned object.

```ruby
params = ActionController::Parameters.new(id: 42)
# => #<ActionController::Parameters {"id"=>42} permitted: false>
params.permit(:id)
# => #<ActionController::Parameters {"id"=>42} permitted: true>
params.expect(:id)
# => 42
```

### `params.require`

It cannot be determined whether `expect(:ids)` or `expect(ids: [])` should be used for the parameter.

`ids` is `42`:

```ruby
params = ActionController::Parameters.new(ids: 42)
# => #<ActionController::Parameters {"ids"=>42} permitted: false>
params.require(:ids)
# => 42
params.expect(:ids)
# => 42
params.expect(ids: [])
# => param is missing or the value is empty or invalid: ids (ActionController::ParameterMissing)
```

`ids` is `[42, 43]`:

```ruby
params = ActionController::Parameters.new(ids: [42, 43])
# => #<ActionController::Parameters {"ids"=>[42, 43]} permitted: false>
params.require(:ids)
# => [42, 43]
params.expect(:ids)
# => param is missing or the value is empty or invalid: ids (ActionController::ParameterMissing)
params.expect(ids: [])
# => [42, 43]
```

### `params[]` and `params.fetch`

Incompatibilities occur when the value is an array.

```ruby
params = ActionController::Parameters.new(ids: [42, 43])
# => #<ActionController::Parameters {"ids"=>[42, 43]} permitted: false>
params[:ids]
# => [42, 43]
params.fetch(:ids)
# => [42, 43]
params.expect(:ids)
# => param is missing or the value is empty or invalid: ids (ActionController::ParameterMissing)
```

These may be designed and provided separately in the future.

Closes rubocop#1358.
koic added a commit to koic/rubocop-rails that referenced this issue Jan 17, 2025
## Summary

This PR adds new `Rails/StrongParametersExpect` cop that enforces
the use of `ActionController::Parameters#expect` as a method for strong parameter handling.

As a starting point for this cop, the implementation in this PR will detect the following cases.

```ruby
# bad
params.require(:user).permit(:name, :age)
params.permit(user: [:name, :age]).require(:user)

# good
params.expect(user: [:name, :age])
```

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

## Additional Information

This cop does not detect the following cases for the reasons outlined below.
Consideration will be given to whether these should be provided as separate options.

### `params.permit`

Incompatibilities occur with the returned object.

```ruby
params = ActionController::Parameters.new(id: 42)
# => #<ActionController::Parameters {"id"=>42} permitted: false>
params.permit(:id)
# => #<ActionController::Parameters {"id"=>42} permitted: true>
params.expect(:id)
# => 42
```

### `params.require`

It cannot be determined whether `expect(:ids)` or `expect(ids: [])` should be used for the parameter.

`ids` is `42`:

```ruby
params = ActionController::Parameters.new(ids: 42)
# => #<ActionController::Parameters {"ids"=>42} permitted: false>
params.require(:ids)
# => 42
params.expect(:ids)
# => 42
params.expect(ids: [])
# => param is missing or the value is empty or invalid: ids (ActionController::ParameterMissing)
```

`ids` is `[42, 43]`:

```ruby
params = ActionController::Parameters.new(ids: [42, 43])
# => #<ActionController::Parameters {"ids"=>[42, 43]} permitted: false>
params.require(:ids)
# => [42, 43]
params.expect(:ids)
# => param is missing or the value is empty or invalid: ids (ActionController::ParameterMissing)
params.expect(ids: [])
# => [42, 43]
```

### `params[]` and `params.fetch`

Incompatibilities occur when the value is an array.

```ruby
params = ActionController::Parameters.new(ids: [42, 43])
# => #<ActionController::Parameters {"ids"=>[42, 43]} permitted: false>
params[:ids]
# => [42, 43]
params.fetch(:ids)
# => [42, 43]
params.expect(:ids)
# => param is missing or the value is empty or invalid: ids (ActionController::ParameterMissing)
```

These may be designed and provided separately in the future.

Closes rubocop#1358.
koic added a commit to koic/rubocop-rails that referenced this issue Jan 17, 2025
## Summary

This PR adds new `Rails/StrongParametersExpect` cop that enforces
the use of `ActionController::Parameters#expect` as a method for strong parameter handling.

As a starting point for this cop, the implementation in this PR will detect the following cases.

```ruby
# bad
params.require(:user).permit(:name, :age)
params.permit(user: [:name, :age]).require(:user)

# good
params.expect(user: [:name, :age])
```

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

## Additional Information

This cop does not detect the following cases for the reasons outlined below.
Consideration will be given to whether these should be provided as separate options.

### `params.permit`

Incompatibilities occur with the returned object.

```ruby
params = ActionController::Parameters.new(id: 42)
# => #<ActionController::Parameters {"id"=>42} permitted: false>
params.permit(:id)
# => #<ActionController::Parameters {"id"=>42} permitted: true>
params.expect(:id)
# => 42
```

### `params.require`

It cannot be determined whether `expect(:ids)` or `expect(ids: [])` should be used for the parameter.

`ids` is `42`:

```ruby
params = ActionController::Parameters.new(ids: 42)
# => #<ActionController::Parameters {"ids"=>42} permitted: false>
params.require(:ids)
# => 42
params.expect(:ids)
# => 42
params.expect(ids: [])
# => param is missing or the value is empty or invalid: ids (ActionController::ParameterMissing)
```

`ids` is `[42, 43]`:

```ruby
params = ActionController::Parameters.new(ids: [42, 43])
# => #<ActionController::Parameters {"ids"=>[42, 43]} permitted: false>
params.require(:ids)
# => [42, 43]
params.expect(:ids)
# => param is missing or the value is empty or invalid: ids (ActionController::ParameterMissing)
params.expect(ids: [])
# => [42, 43]
```

### `params[]` and `params.fetch`

Incompatibilities occur when the value is an array.

```ruby
params = ActionController::Parameters.new(ids: [42, 43])
# => #<ActionController::Parameters {"ids"=>[42, 43]} permitted: false>
params[:ids]
# => [42, 43]
params.fetch(:ids)
# => [42, 43]
params.expect(:ids)
# => param is missing or the value is empty or invalid: ids (ActionController::ParameterMissing)
```

These may be designed and provided separately in the future.

Closes rubocop#1358.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Request for new functionality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants