Skip to content

Expose an invert_where method that will invert all scope conditions.#40249

Merged
rafaelfranca merged 1 commit intorails:masterfrom
kddnewton:invert-where
Dec 18, 2020
Merged

Expose an invert_where method that will invert all scope conditions.#40249
rafaelfranca merged 1 commit intorails:masterfrom
kddnewton:invert-where

Conversation

@kddnewton
Copy link
Contributor

Came up at work that there was a complicated scope that someone was trying to invert and couldn't think of a clean way without touching internals. Was happy to see there is already an invert method present since it's used for where.not. This just exposes that method.

@simi
Copy link
Contributor

simi commented Sep 18, 2020

I really like this idea. Few notes after initial thinking:

  • If I understand it well, this mutates original scope and should be bang method.
  • Would it make sense to expose also non-mutating alternative?
  • Since there's already WhereChain, what about to use it directly like scope.where.invert!?

@kddnewton
Copy link
Contributor Author

@simi yeah those all make sense. The inspiration for this was based on reverse_order, which does a very similar thing here. (Though tbh maybe that should be .order.reverse).

Do you mean a non-mutating alternative as in something that returns a new relation? I think I'd rather only expose new API. I'd be fine with it if it were a non-mutating one though that spawned a new relation with the inverted where.

@rails-bot
Copy link

rails-bot bot commented Dec 17, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label Dec 17, 2020
Comment on lines 711 to 719
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
self.where_clause = where_clause.invert
self
scope = spawn
scope.where_clause = where_clause.invert
scope

We don't change relation inline for public API methods in Relation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure! I'll fix that.

While I'm here, would you prefer invert_where or where.invert? I saw where.missing and where.associated made it in, so maybe that would fit better.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it will not take any argument I'd prefer invert_where

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated! I used the existing examples to do an invert_where that does spawn.invert_where! and an invert_where! that modifies in place.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it will not take any argument I'd prefer invert_where

That would also match rewhere. It's closer to manipulating the where clause itself than doing extra operations or expressions on it, like where.not etc.

@rails-bot rails-bot bot removed the stale label Dec 17, 2020
@rafaelfranca
Copy link
Member

Can you rebase the PR? It has conflicts.

```ruby
class User
  scope :active, -> { where(accepted: true, locked: false) }
end

User.active

User.active.invert_where
```
@kddnewton
Copy link
Contributor Author

@rafaelfranca rebased

@rafaelfranca rafaelfranca merged commit 23019c3 into rails:master Dec 18, 2020
@brandonzylstra
Copy link
Contributor

where takes a hash of keys and values, so anything with *_where would seem like it ought to do the same. But in this case, if I'm not mistaken, it is essentially short for invert_the_where_clause.

Wouldn't invert_scope or just invert be much cleaner and more intuitive?

@ezekg
Copy link
Contributor

ezekg commented Apr 26, 2021

This method seem to be an incredibly dangerous footgun. What if the where clause had a current account or user scope on it that the developer wasn't immediately aware of? This would invert that scope and show all records not associated with that user, potentially leaking cross-tenant records. I don't really think this is a wise change.

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.

I think this could be resolved by explicitly taking in the clauses to invert, rather than inverting everything for the current scope. Regardless, I feel like where.not is more explicit and much clearer at first-glance.

@why-el
Copy link
Contributor

why-el commented Mar 22, 2025

In addition to the scope issue Zeke proves above, there is another pretty substantial issue. Reversing where conditions in SQL is not always logically equivalent, especially if there are NULL values. So if you run invert_where, you could get unexpected results.

An example:

management-app(dev)> Foo.count
  Foo Count (0.2ms)  SELECT COUNT(*) FROM "foos" /*application='ManagementApp'*/
=> 4
management-app(dev)> Foo.all.map {|e| e.value }
=> [1, 2, 3, nil]
management-app(dev)> Foo.where(value: 1)
=> [#<Foo:0x0000ffff8bee3848 id: 1, value: 1, created_at: "2025-03-21 13:46:37.413956000 +0000", updated_at: "2025-03-21 13:46:37.413956000 +0000">]
management-app(dev)> Foo.where(value: 1).invert_where
  Foo Load (1.8ms)  SELECT "foos".* FROM "foos" WHERE "foos"."value" != 1 /* loading for pp */ LIMIT 11 /*application='ManagementApp'*/
=> 
[#<Foo:0x0000ffff8beed348 id: 2, value: 2, created_at: "2025-03-21 13:46:47.227245000 +0000", updated_at: "2025-03-21 13:46:47.227245000 +0000">,
 #<Foo:0x0000ffff8beed208 id: 3, value: 3, created_at: "2025-03-21 13:46:50.207437000 +0000", updated_at: "2025-03-21 13:46:50.207437000 +0000">]
management-app(dev)> 

Foo model has four rows, 1, 2, 3, and null. If I say give me value 1, I get that record. But if I say give me all that is not 1, that is, I invert that where, I only get 2 and 3, the row with null value is not there. This is because logical inversions of the where clause were assumed to be data-independent, but there are not at all.

This is a famous issue with the incomparability of NULL values, and it is the first example that came to my mind when I saw this feature in the Rails Weekly mailing list. Of course, this is no different than if one just did Foo.where.not(value: 1), that one will face the same issue, but I think this particular new DSL (invert_where) might exacerbate or proliferate the problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants