rubocop / rubocop-rails

A RuboCop extension focused on enforcing Rails best practices and coding conventions.
https://docs.rubocop.org/rubocop-rails
MIT License
819 stars 263 forks source link

New cop request for invert_where which will be introduced since Rails 7 #470

Open pocke opened 3 years ago

pocke commented 3 years ago

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

Rails 7 will introduce AR::Relation#invert_where method. https://github.com/rails/rails/pull/40249

But it seems dangerous in many cases. Because the method inverts all where clauses implicitly. For example, where(a: 1).where(b: 1).invert_where means WHERE NOT (a = 1 AND b = 1). You can try this behavior with this code snippet. https://gist.github.com/pocke/6c39b2eb215cdc117b996c987dc605ab

So, I think RuboCop-Rails should assist to use invert_where safely.

I have the following three examples that could be a bug.

First, if invert_where is used in a scope definition, it introduces bugs.

class User < ApplicationRecrod
  scope :alive, -> { where(deleted_at: nil) }
  scope :dead -> { alive.invert_where }       # <- It is the problem

  scope :admin, -> { where(role: :admin) }
end

User.alive # ok
User.dead # ok

# The "dead" scope also inverts `admin` scope, so it means `WHERE NOT (role = 'admin' AND deleted_at IS NULL)`.
# But it is not expected
User.admin.dead

Second, if the invert_where is used for a relation that is created in another place, it can invert unexpected where clauses. The problem is described in this comment. https://github.com/rails/rails/pull/40249#issuecomment-826881180

Imagine, somewhere within the request flow, perhaps automatically scoped by Pundit,

current_user = User.first
posts = Post.where(author: current_user)

And then later on in a controller…

published_posts = posts.where(published: true)
draft_posts = published_posts.invert_where

The draft_posts variable is now all draft posts NOT by the current user. Putting the above logic in published and unpublished scopes, respectively, would be make this issue even more confusing to debug if it did occur.

Third, it inverts default_scope also.

class User < ApplicationRecord
  default_scope -> { where(secret: false) }

  scope :alive, -> { where(deleted_at: nil) }
end

User.alive.invert_where # It also inverts the default_scope, so it means `WHERE NOT (secret = false AND deleted_at IS NULL)`

Describe the solution you'd like

We need a cop to detect dangerous usage of the method.

I have two ideas.

FIrst, warn invert_where in a scope definition. For example:

class User < ApplicationRecord
  scope :dead, -> { alive.invert_value } # <- bad
end

User.something.invert_where # <- good

users = User.something
users.something2.invert_value # <- This cop doesn't warn it, but it can be a problem.

This solution avoids the first problem, but it isn't aware of the other problems.

Second, warn invert_where if the method chain doesn't start with a constant. For example:

class User < ApplicationRecord
  scope :dead, -> { alive.invert_value } # <- bad
end

User.something.invert_where # <- good

User.something.something2.invert_where # <- good

# bad - because inverting `something` scope is probably a mistake.
users = User.something
users.something2.invert_value

This solution avoids the first and second problems, but the third, default_scope, problem is not warned.

I think avoiding the default_scope problem by RuboCop is difficult. If RuboCop warns all usage of invert_where we can avoid the default_scope problem, but I feel it is a too strong rule.

Describe alternatives you've considered

Nothing else.

Additional context

Rails 7 has not released yet, so invert_where's behavior may be changed (or reverted) until Rails 7 release. So implementing this cop is too early for now.

dvandersluis commented 3 years ago

Given the linked comment, I think it’d be useful to have a cop option to just disallow invert_where.