From 0a5b41c4c044f3aec9ad3c17ba6149f17697a86d Mon Sep 17 00:00:00 2001 From: Brad Price Date: Mon, 18 Nov 2019 17:02:33 -0600 Subject: [PATCH] Check that entire collection has been loaded before short circuiting Currently, when checking that the collection has been loaded, only the first record is checked. In specific scenarios, if a record is fetched via an `after_initialize` hook, there is a chance that the first record has been loaded, but other records in the collection have not. In order to successfully short circuit the fetching of data, we need to verify that all of the records in the collection have been loaded. * Create test for edge case * Move `reset_callbacks` method to `cases/helper`, since it has been defined in multiple locations. Closes #37730 --- .../active_record/associations/preloader.rb | 2 +- .../test/cases/associations/eager_test.rb | 15 ++++++++++++++ .../has_many_associations_test.rb | 20 +++---------------- .../associations/inverse_associations_test.rb | 7 ------- activerecord/test/cases/test_case.rb | 14 +++++++++++++ activerecord/test/models/comment.rb | 1 + 6 files changed, 34 insertions(+), 25 deletions(-) diff --git a/activerecord/lib/active_record/associations/preloader.rb b/activerecord/lib/active_record/associations/preloader.rb index 5bba9452021c9..e1f4049163d6e 100644 --- a/activerecord/lib/active_record/associations/preloader.rb +++ b/activerecord/lib/active_record/associations/preloader.rb @@ -184,7 +184,7 @@ def records_by_owner # and attach it to a relation. The class returned implements a `run` method # that accepts a preloader. def preloader_for(reflection, owners) - if owners.first.association(reflection.name).loaded? + if owners.all? { |o| o.association(reflection.name).loaded? } return AlreadyLoaded end reflection.check_preloadable! diff --git a/activerecord/test/cases/associations/eager_test.rb b/activerecord/test/cases/associations/eager_test.rb index cb46f9e05368b..579972b82d143 100644 --- a/activerecord/test/cases/associations/eager_test.rb +++ b/activerecord/test/cases/associations/eager_test.rb @@ -626,6 +626,21 @@ def test_eager_with_has_many_through_an_sti_join_model assert_equal [comments(:does_it_hurt)], assert_no_queries { author.special_post_comments } end + def test_preloading_with_has_one_through_an_sti_with_after_initialize + author_a = Author.create!(name: "A") + author_b = Author.create!(name: "B") + post_a = StiPost.create!(author: author_a, title: "TITLE", body: "BODY") + post_b = SpecialPost.create!(author: author_b, title: "TITLE", body: "BODY") + comment_a = SpecialComment.create!(post: post_a, body: "TEST") + comment_b = SpecialComment.create!(post: post_b, body: "TEST") + reset_callbacks(StiPost, :initialize) do + StiPost.after_initialize { author } + comments = SpecialComment.where(id: [comment_a.id, comment_b.id]).includes(:author).to_a + comments_with_author = comments.map { |c| [c.id, c.author.try(:id)] } + assert_equal comments_with_author.size, comments_with_author.map(&:second).compact.size + end + end + def test_preloading_has_many_through_with_implicit_source authors = Author.includes(:very_special_comments).to_a assert_no_queries do diff --git a/activerecord/test/cases/associations/has_many_associations_test.rb b/activerecord/test/cases/associations/has_many_associations_test.rb index 0e6ce32431ea0..a13c5af80502c 100644 --- a/activerecord/test/cases/associations/has_many_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_associations_test.rb @@ -2831,7 +2831,7 @@ def self.name end test "prevent double insertion of new object when the parent association loaded in the after save callback" do - reset_callbacks(:save, Bulb) do + reset_callbacks(Bulb, :save) do Bulb.after_save { |record| record.car.bulbs.load } car = Car.create! @@ -2842,7 +2842,7 @@ def self.name end test "prevent double firing the before save callback of new object when the parent association saved in the callback" do - reset_callbacks(:save, Bulb) do + reset_callbacks(Bulb, :save) do count = 0 Bulb.before_save { |record| record.car.save && count += 1 } @@ -2961,7 +2961,7 @@ def test_ids_reader_memoization end def test_loading_association_in_validate_callback_doesnt_affect_persistence - reset_callbacks(:validation, Bulb) do + reset_callbacks(Bulb, :validation) do Bulb.after_validation { |record| record.car.bulbs.load } car = Car.create!(name: "Car") @@ -2996,18 +2996,4 @@ def test_has_many_preloading_with_duplicate_records def force_signal37_to_load_all_clients_of_firm companies(:first_firm).clients_of_firm.load_target end - - def reset_callbacks(kind, klass) - old_callbacks = {} - old_callbacks[klass] = klass.send("_#{kind}_callbacks").dup - klass.subclasses.each do |subclass| - old_callbacks[subclass] = subclass.send("_#{kind}_callbacks").dup - end - yield - ensure - klass.send("_#{kind}_callbacks=", old_callbacks[klass]) - klass.subclasses.each do |subclass| - subclass.send("_#{kind}_callbacks=", old_callbacks[subclass]) - end - end end diff --git a/activerecord/test/cases/associations/inverse_associations_test.rb b/activerecord/test/cases/associations/inverse_associations_test.rb index 9f5270e58045e..96335c89e1a5b 100644 --- a/activerecord/test/cases/associations/inverse_associations_test.rb +++ b/activerecord/test/cases/associations/inverse_associations_test.rb @@ -545,13 +545,6 @@ def test_inverse_instance_should_be_set_before_initialize_callbacks_are_run assert_predicate Man.joins(:interests).includes(:interests).first.interests, :any? end end - - def reset_callbacks(target, type) - old_callbacks = target.send(:get_callbacks, type).deep_dup - yield - ensure - target.send(:set_callbacks, type, old_callbacks) if old_callbacks - end end class InverseBelongsToTests < ActiveRecord::TestCase diff --git a/activerecord/test/cases/test_case.rb b/activerecord/test/cases/test_case.rb index 9696d407d8b17..43eb96de5152b 100644 --- a/activerecord/test/cases/test_case.rb +++ b/activerecord/test/cases/test_case.rb @@ -87,6 +87,20 @@ def with_has_many_inversing ensure ActiveRecord::Base.has_many_inversing = old end + + def reset_callbacks(klass, kind) + old_callbacks = {} + old_callbacks[klass] = klass.send("_#{kind}_callbacks").dup + klass.subclasses.each do |subclass| + old_callbacks[subclass] = subclass.send("_#{kind}_callbacks").dup + end + yield + ensure + klass.send("_#{kind}_callbacks=", old_callbacks[klass]) + klass.subclasses.each do |subclass| + subclass.send("_#{kind}_callbacks=", old_callbacks[subclass]) + end + end end class PostgreSQLTestCase < TestCase diff --git a/activerecord/test/models/comment.rb b/activerecord/test/models/comment.rb index f0f057670957f..2b8ef5eb89984 100644 --- a/activerecord/test/models/comment.rb +++ b/activerecord/test/models/comment.rb @@ -59,6 +59,7 @@ def to_s end class SpecialComment < Comment + has_one :author, through: :post default_scope { where(deleted_at: nil) } def self.what_are_you