From 33d62981b667e72e25c38de848dbac843713f192 Mon Sep 17 00:00:00 2001 From: Ryuta Kamizono Date: Sun, 14 Aug 2016 21:38:33 +0900 Subject: `CollectionProxy#take` should respect dirty target `#first`, `#second`, ..., `#last` methods respects dirty target. But `#take` doesn't respect it. This commit fixes the inconsistent behavior. --- .../associations/collection_association.rb | 22 +++++++++------------- .../active_record/associations/collection_proxy.rb | 4 ++-- .../associations/has_many_associations_test.rb | 12 ++++++++---- 3 files changed, 19 insertions(+), 19 deletions(-) (limited to 'activerecord') diff --git a/activerecord/lib/active_record/associations/collection_association.rb b/activerecord/lib/active_record/associations/collection_association.rb index ab267f6897..44404cc176 100644 --- a/activerecord/lib/active_record/associations/collection_association.rb +++ b/activerecord/lib/active_record/associations/collection_association.rb @@ -147,11 +147,11 @@ module ActiveRecord first_nth_or_last(:last, *args) end - def take(n = nil) - if loaded? - n ? target.take(n) : target.first + def take(limit = nil) + if find_from_target? + limit ? load_target.take(limit) : load_target.first else - scope.take(n) + scope.take(limit) end end @@ -608,14 +608,10 @@ module ActiveRecord # * target already loaded # * owner is new record # * target contains new or changed record(s) - def fetch_first_nth_or_last_using_find?(args) - if args.first.is_a?(Hash) - true - else - !(loaded? || - owner.new_record? || - target.any? { |record| record.new_record? || record.changed? }) - end + def find_from_target? + loaded? || + owner.new_record? || + target.any? { |record| record.new_record? || record.changed? } end def include_in_memory?(record) @@ -649,7 +645,7 @@ module ActiveRecord def first_nth_or_last(type, *args) args.shift if args.first.is_a?(Hash) && args.first.empty? - collection = fetch_first_nth_or_last_using_find?(args) ? scope : load_target + collection = find_from_target? ? load_target : scope collection.send(type, *args) end end diff --git a/activerecord/lib/active_record/associations/collection_proxy.rb b/activerecord/lib/active_record/associations/collection_proxy.rb index 806a905323..fb1b31429e 100644 --- a/activerecord/lib/active_record/associations/collection_proxy.rb +++ b/activerecord/lib/active_record/associations/collection_proxy.rb @@ -262,8 +262,8 @@ module ActiveRecord # another_person_without.pets # => [] # another_person_without.pets.take # => nil # another_person_without.pets.take(2) # => [] - def take(n = nil) - @association.take(n) + def take(limit = nil) + @association.take(limit) end # Returns a new object of the collection type that has been instantiated diff --git a/activerecord/test/cases/associations/has_many_associations_test.rb b/activerecord/test/cases/associations/has_many_associations_test.rb index 09692fc3a0..431ac9ff94 100644 --- a/activerecord/test/cases/associations/has_many_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_associations_test.rb @@ -525,14 +525,18 @@ class HasManyAssociationsTest < ActiveRecord::TestCase def test_taking_with_a_number # taking from unloaded Relation bob = Author.find(authors(:bob).id) + new_post = bob.posts.build + assert_not bob.posts.loaded? assert_equal [posts(:misc_by_bob)], bob.posts.take(1) - bob = Author.find(authors(:bob).id) assert_equal [posts(:misc_by_bob), posts(:other_by_bob)], bob.posts.take(2) + assert_equal [posts(:misc_by_bob), posts(:other_by_bob), new_post], bob.posts.take(3) # taking from loaded Relation - bob.posts.to_a - assert_equal [posts(:misc_by_bob)], authors(:bob).posts.take(1) - assert_equal [posts(:misc_by_bob), posts(:other_by_bob)], authors(:bob).posts.take(2) + bob.posts.load + assert bob.posts.loaded? + assert_equal [posts(:misc_by_bob)], bob.posts.take(1) + assert_equal [posts(:misc_by_bob), posts(:other_by_bob)], bob.posts.take(2) + assert_equal [posts(:misc_by_bob), posts(:other_by_bob), new_post], bob.posts.take(3) end def test_taking_with_inverse_of -- cgit v1.2.3