Skip to content

Commit

Permalink
Fix unscoping association scope on joins not to raise an error
Browse files Browse the repository at this point in the history
#29589 changed merging scope order to allow to unscope default scopes on
association scope (#29611), but that caused a regression #38811 that
accidentally allow join constraint which is required.

```
% bin/test test/cases/associations/has_many_associations_test.rb -n test_unscopes_the_default_scope_of_associated_model_when_used_with_include
Run options: -n test_unscopes_the_default_scope_of_associated_model_when_used_with_include --seed 32978

# Running:

E

Error:
HasManyAssociationsTest#test_unscopes_the_default_scope_of_associated_model_when_used_with_include:
NoMethodError: undefined method `children' for nil:NilClass
    ~/rails/activerecord/lib/active_record/associations/join_dependency/join_association.rb:39:in `block in join_constraints'
    ~/rails/activerecord/lib/active_record/associations/join_dependency/join_association.rb:30:in `reverse_each'
    ~/rails/activerecord/lib/active_record/associations/join_dependency/join_association.rb:30:in `with_index'
    ~/rails/activerecord/lib/active_record/associations/join_dependency/join_association.rb:30:in `join_constraints'
    ~/rails/activerecord/lib/active_record/associations/join_dependency.rb:171:in `make_constraints'
    ~/rails/activerecord/lib/active_record/associations/join_dependency.rb:196:in `block in walk'
    ~/rails/activerecord/lib/active_record/associations/join_dependency.rb:196:in `each'
    ~/rails/activerecord/lib/active_record/associations/join_dependency.rb:196:in `flat_map'
    ~/rails/activerecord/lib/active_record/associations/join_dependency.rb:196:in `walk'
    ~/rails/activerecord/lib/active_record/associations/join_dependency.rb:90:in `block in join_constraints'
    ~/rails/activerecord/lib/active_record/associations/join_dependency.rb:87:in `each'
    ~/rails/activerecord/lib/active_record/associations/join_dependency.rb:87:in `flat_map'
    ~/rails/activerecord/lib/active_record/associations/join_dependency.rb:87:in `join_constraints'
    ~/rails/activerecord/lib/active_record/relation/query_methods.rb:1226:in `build_join_query'
    ~/rails/activerecord/lib/active_record/relation/query_methods.rb:1211:in `build_joins'
    ~/rails/activerecord/lib/active_record/relation/query_methods.rb:1091:in `build_arel'
    ~/rails/activerecord/lib/active_record/relation/query_methods.rb:1063:in `arel'
    ~/rails/activerecord/lib/active_record/relation/finder_methods.rb:419:in `block in limited_ids_for'
    ~/rails/activerecord/lib/active_record/relation.rb:867:in `skip_query_cache_if_necessary'
    ~/rails/activerecord/lib/active_record/relation/finder_methods.rb:419:in `limited_ids_for'
    ~/rails/activerecord/lib/active_record/relation/finder_methods.rb:398:in `apply_join_dependency'
    ~/rails/activerecord/lib/active_record/relation.rb:839:in `block in exec_queries'
    ~/rails/activerecord/lib/active_record/relation.rb:867:in `skip_query_cache_if_necessary'
    ~/rails/activerecord/lib/active_record/relation.rb:834:in `exec_queries'
    ~/rails/activerecord/lib/active_record/relation.rb:639:in `load'
    ~/rails/activerecord/lib/active_record/relation.rb:250:in `records'
    ~/rails/activerecord/lib/active_record/relation/finder_methods.rb:508:in `find_take'
    ~/rails/activerecord/lib/active_record/relation/finder_methods.rb:98:in `take'
    ~/rails/activerecord/lib/active_record/relation/finder_methods.rb:458:in `find_one'
    ~/rails/activerecord/lib/active_record/relation/finder_methods.rb:442:in `find_with_ids'
    ~/rails/activerecord/lib/active_record/relation/finder_methods.rb:69:in `find'
    ~/rails/activerecord/test/cases/associations/has_many_associations_test.rb:2689:in `block in <class:HasManyAssociationsTest>'

bin/test test/cases/associations/has_many_associations_test.rb:2683
```

Required join constraint should not be allowed to unscoping.

Fixes #38811.
  • Loading branch information
kamipo committed Apr 15, 2020
1 parent 6f2126c commit e93f416
Show file tree
Hide file tree
Showing 3 changed files with 17 additions and 12 deletions.
12 changes: 7 additions & 5 deletions activerecord/lib/active_record/reflection.rb
Original file line number Diff line number Diff line change
Expand Up @@ -183,20 +183,22 @@ def join_scope(table, foreign_table, foreign_klass)
scope_chain_items = join_scopes(table, predicate_builder)
klass_scope = klass_join_scope(table, predicate_builder)

if type
klass_scope.where!(type => foreign_klass.polymorphic_name)
end

scope_chain_items.inject(klass_scope, &:merge!)

key = join_keys.key
foreign_key = join_keys.foreign_key

klass_scope.where!(table[key].eq(foreign_table[foreign_key]))

if type
klass_scope.where!(type => foreign_klass.polymorphic_name)
end

if klass.finder_needs_type_condition?
klass_scope.where!(klass.send(:type_condition, table))
end

scope_chain_items.inject(klass_scope, &:merge!)
klass_scope
end

def join_scopes(table, predicate_builder) # :nodoc:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2658,10 +2658,11 @@ def test_association_with_rewhere_doesnt_set_inverse_instance_key

assert_equal [bulb1], car.bulbs
assert_equal [bulb1, bulb2], car.all_bulbs.sort_by(&:id)
assert_equal [bulb1, bulb2], Car.includes(:all_bulbs).find(car.id).all_bulbs.sort_by(&:id)
assert_equal [bulb1, bulb2], Car.eager_load(:all_bulbs).find(car.id).all_bulbs.sort_by(&:id)
end

test "can unscope and where the default scope of the associated model" do
Car.has_many :other_bulbs, -> { unscope(where: [:name]).where(name: "other") }, class_name: "Bulb"
car = Car.create!
bulb1 = Bulb.create! name: "defaulty", car: car
bulb2 = Bulb.create! name: "other", car: car
Expand All @@ -2671,7 +2672,6 @@ def test_association_with_rewhere_doesnt_set_inverse_instance_key
end

test "can rewhere the default scope of the associated model" do
Car.has_many :old_bulbs, -> { rewhere(name: "old") }, class_name: "Bulb"
car = Car.create!
bulb1 = Bulb.create! name: "defaulty", car: car
bulb2 = Bulb.create! name: "old", car: car
Expand All @@ -2682,11 +2682,11 @@ def test_association_with_rewhere_doesnt_set_inverse_instance_key

test "unscopes the default scope of associated model when used with include" do
car = Car.create!
bulb = Bulb.create! name: "other", car: car
bulb1 = Bulb.create! name: "defaulty", car: car
bulb2 = Bulb.create! name: "other", car: car

assert_equal [bulb], Car.find(car.id).all_bulbs
assert_equal [bulb], Car.includes(:all_bulbs).find(car.id).all_bulbs
assert_equal [bulb], Car.eager_load(:all_bulbs).find(car.id).all_bulbs
assert_equal [bulb1, bulb2], Car.includes(:all_bulbs2).find(car.id).all_bulbs2.sort_by(&:id)
assert_equal [bulb1, bulb2], Car.eager_load(:all_bulbs2).find(car.id).all_bulbs2.sort_by(&:id)
end

test "raises RecordNotDestroyed when replaced child can't be destroyed" do
Expand Down
5 changes: 4 additions & 1 deletion activerecord/test/models/car.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,10 @@

class Car < ActiveRecord::Base
has_many :bulbs
has_many :all_bulbs, -> { unscope where: :name }, class_name: "Bulb"
has_many :all_bulbs, -> { unscope(where: :name) }, class_name: "Bulb"
has_many :all_bulbs2, -> { unscope(:where) }, class_name: "Bulb"
has_many :other_bulbs, -> { unscope(where: :name).where(name: "other") }, class_name: "Bulb"
has_many :old_bulbs, -> { rewhere(name: "old") }, class_name: "Bulb"
has_many :funky_bulbs, class_name: "FunkyBulb", dependent: :destroy
has_many :failed_bulbs, class_name: "FailedBulb", dependent: :destroy
has_many :foo_bulbs, -> { where(name: "foo") }, class_name: "Bulb"
Expand Down

0 comments on commit e93f416

Please sign in to comment.