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
Enable scoping to apply to all queries #41397
Merged
eileencodes
merged 1 commit into
rails:main
from
eileencodes:allow-scoping-to-apply-to-all-queries
Feb 11, 2021
Merged
Enable scoping to apply to all queries #41397
eileencodes
merged 1 commit into
rails:main
from
eileencodes:allow-scoping-to-apply-to-all-queries
Feb 11, 2021
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
eileencodes
force-pushed
the
allow-scoping-to-apply-to-all-queries
branch
from
February 10, 2021 19:48
5a0ce09
to
143af37
Compare
rafaelfranca
approved these changes
Feb 10, 2021
eileencodes
force-pushed
the
allow-scoping-to-apply-to-all-queries
branch
2 times, most recently
from
February 10, 2021 19:56
c201676
to
0bbec24
Compare
kamipo
reviewed
Feb 11, 2021
```ruby | ||
Post.where(blog_id: post.blog_id).scoping(all_queries: true) do | ||
post.update(title: "a post title") # adds `posts.blog_id = 1` to the query | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggested change
end | |
end | |
``` |
@@ -99,6 +99,8 @@ def target=(target) | |||
def scope | |||
if (scope = klass.current_scope) && scope.try(:proxy_association) == self | |||
scope.spawn | |||
elsif (scope = klass.global_current_scope) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggested change
elsif (scope = klass.global_current_scope) | |
elsif scope = klass.global_current_scope |
eileencodes
force-pushed
the
allow-scoping-to-apply-to-all-queries
branch
2 times, most recently
from
February 11, 2021 21:03
5e66367
to
e4dd1ca
Compare
Similar to rails#40720 and rails#40805 this change allows for the `scoping` method to apply to all queries in the block. Previously this would only apply to queries on the class and not the instance. Ie `Post.create`, Post.all`, but not `post.update`, or `post.delete`. The change here will create a global scope that is applied to all queries for a relation for the duration of the block. Benefits: This change allows applications to add a scope to any query for the duration of a block. This is useful for applications using sharding to be able to control the query without requiring a `default_scope`. This is useful if you want to have more control over when a `scoping` is used on a relation. This also brings `scoping` in parity with the behavior of `default_scope` so there are less surprises between the behavior of these two methods. There are a caveats to this behavior: 1) The `scoping` only applies to objects of the same type. IE you cannot scope `Post.where(blog_id: 1).scoping` and then expect `post.comments` will apply `blog_id = 1` to the `Comment` query. This is not possible because the scope is `posts.blog_id = 1` and we can't apply the `posts` scope to a `comments` query. To solve this, scopes must be nested. 2) If a block is scoped to `all_queries` it cannot be unscoped without exiting the block. I couldn't find a way around this but ActiveRecord scoping is a bit complex and turning off `all_queries` when it's already on in nested scoping blocks had interesting behavior that I decided was best left out.
eileencodes
force-pushed
the
allow-scoping-to-apply-to-all-queries
branch
from
February 11, 2021 21:04
e4dd1ca
to
d6e12ff
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Similar to #40720
and #40805 this change allows for the
scoping
method to apply to all queries in the block. Previously thiswould only apply to queries on the class and not the instance. Ie
Post.create
,Post.all
, but notpost.update
, orpost.delete
.The change here will create a global scope that is applied to all
queries for a relation for the duration of the block.
Benefits:
This change allows applications to add a scope to any query for the
duration of a block. This is useful for applications using sharding to
be able to control the query without requiring a
default_scope
. Thisis useful if you want to have more control over when a
scoping
is usedon a relation. This also brings
scoping
in parity with the behavior ofdefault_scope
so there are less surprises between the behavior ofthese two methods.
There are a caveats to this behavior:
scoping
only applies to objects of the same type. IE you cannotscope
Post.where(blog_id: 1).scoping
and then expectpost.comments
will apply
blog_id = 1
to theComment
query. This is not possiblebecause the scope is
posts.blog_id = 1
and we can't apply theposts
scope to a
comments
query. To solve this, scopes must be nested.all_queries
it cannot be unscoped withoutexiting the block. I couldn't find a way around this but ActiveRecord
scoping is a bit complex and turning off
all_queries
when it's alreadyon in nested scoping blocks had interesting behavior that I decided was
best left out.