Skip to content

Commit

Permalink
Post.joins(:users) should not be affected by User.current_scope
Browse files Browse the repository at this point in the history
This change was introduced by rails#18109. The intent of that change was to
specifically apply `unscoped`, not to allow all changes to
`current_scope` to affect the join. The idea of allowing `current_scope`
to affect joins is interesting and potentially more consistent, but has
sever problems associated with it. The fact that we're specifically
stripping out joins indicates one such problem (and potentially leads to
invalid queries).

Ultimately it's difficult to reason about what `Posts.joins(:users)`
actually means if it's affected by `User.current_scope`, and it's
difficult to specifically control what does or doesn't get added. If we
were starting from scratch, I don't think I'd have `joins` be affected
by `default_scope` either, but that's too big of a breaking change to
make at this point.

With this change, we no longer apply `current_scope` when bringing in
joins, with the singular exception of the motivating use case which
introduced this bug, which is providing a way to *opt-out* of having the
default scope apply to joins.

Fixes rails#29338.
  • Loading branch information
sgrif committed Jul 17, 2017
1 parent 1dc2344 commit 3630d63
Show file tree
Hide file tree
Showing 3 changed files with 19 additions and 4 deletions.
7 changes: 7 additions & 0 deletions activerecord/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,10 @@
* `Relation#joins` is no longer affected by the target model's
`current_scope`, with the exception of `unscoped`.

Fixes #29338.

*Sean Griffin*

* Previously, when building records using a `has_many :through` association,
if the child records were deleted before the parent was saved, they would
still be persisted. Now, if child records are deleted before the parent is saved
Expand Down
6 changes: 2 additions & 4 deletions activerecord/lib/active_record/reflection.rb
Original file line number Diff line number Diff line change
Expand Up @@ -197,10 +197,8 @@ def join_scopes(table, predicate_builder) # :nodoc:
end

def klass_join_scope(table, predicate_builder) # :nodoc:
if klass.current_scope
klass.current_scope.clone.tap { |scope|
scope.joins_values = scope.left_outer_joins_values = [].freeze
}
if klass.current_scope && klass.current_scope.values.empty?
klass.unscoped
else
relation = ActiveRecord::Relation.create(
klass,
Expand Down
10 changes: 10 additions & 0 deletions activerecord/test/cases/scoping/default_scoping_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -372,6 +372,16 @@ def test_default_scope_with_joins
Comment.joins(:post).count
end

def test_joins_not_affected_by_scope_other_than_default_or_unscoped
without_scope_on_post = Comment.joins(:post).to_a
with_scope_on_post = nil
Post.where(id: [1, 5, 6]).scoping do
with_scope_on_post = Comment.joins(:post).to_a
end

assert_equal with_scope_on_post, without_scope_on_post
end

def test_unscoped_with_joins_should_not_have_default_scope
assert_equal SpecialPostWithDefaultScope.unscoped { Comment.joins(:special_post_with_default_scope).to_a },
Comment.joins(:post).to_a
Expand Down

0 comments on commit 3630d63

Please sign in to comment.