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

Describe unsafe usage of invert_where method [ci skip] #42118


Copy link

@pocke pocke commented May 1, 2021

invert_where method has been added by #40249.
This pull request adds documentation of unsafe usage of invert_where.

As #40249 (comment), this method is dangerous.
Because it inverts all conditions before invert_where call.

For example

require "bundler/inline"

gemfile(true) do
  source ""

  git_source(:github) { |repo| "{repo}.git" }

  gem "activerecord", github: 'rails/rails', ref: 'be630f7d1d948c274bbb67ab6ed2148c539c9b8c'
  gem "activemodel", github: 'rails/rails', ref: 'be630f7d1d948c274bbb67ab6ed2148c539c9b8c'
  gem "activesupport", github: 'rails/rails', ref: 'be630f7d1d948c274bbb67ab6ed2148c539c9b8c'
  gem "sqlite3"

require "active_record"

ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")

ActiveRecord::Schema.define do
  create_table :users, force: true do |t|
    t.string :role
    t.boolean :accepted
    t.boolean :locked

class User < ActiveRecord::Base
  scope :active, -> { where(accepted: true, locked: false) }
  scope :inactive, -> { active.invert_where } # Do not attempt it
puts User.where(role: 'admin').inactive.to_sql
# SELECT "users".* FROM "users" WHERE NOT ("users"."role" = 'admin' AND "users"."accepted" = 1 AND "users"."locked" = 0)

On the last line, User.where(role: 'admin').inactive inverts where(role: 'admin') unexpectedly.

But the current documentation only describes a single where method call case. So we can understand how where(...).invert_where works, but cannot understand how where(...).where(...).invert_where works from the document.

This behavior can become a bug, so the documentation should describe the behavior.

By the way, I'm wondering if adding it should be reverted. Because it is too dangerous IMO. I will not use this method for my project to avoid problems.
But I think it needs the document to describe the risk at least.

I also opened an issue to rubocop-rails for this method. rubocop/rubocop-rails#470

@zzak zzak merged commit 75edde0 into rails:main May 2, 2021
@pocke pocke deleted the Describe_unsafe_usage_of__invert_where__method__ci_skip_ branch May 2, 2021 02:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet

Successfully merging this pull request may close these issues.

None yet

2 participants