From 788eb51df3007d94b266965433b0be88a23b84c5 Mon Sep 17 00:00:00 2001 From: Yuya Tanaka Date: Wed, 6 Feb 2019 12:29:28 +0900 Subject: Fix `CollectionProxy#concat` to return self by alias it to `#<<` Formerly it was returning arguments (`records` array). --- .../associations/collection_association.rb | 5 ++-- .../active_record/associations/collection_proxy.rb | 34 +++------------------- .../associations/has_many_associations_test.rb | 3 +- .../has_many_through_associations_test.rb | 3 +- 4 files changed, 10 insertions(+), 35 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 6f5df807fe..68f53d5c1c 100644 --- a/activerecord/lib/active_record/associations/collection_association.rb +++ b/activerecord/lib/active_record/associations/collection_association.rb @@ -109,9 +109,8 @@ module ActiveRecord end end - # Add +records+ to this association. Returns +self+ so method calls may - # be chained. Since << flattens its argument list and inserts each record, - # +push+ and +concat+ behave identically. + # Add +records+ to this association. Since +<<+ flattens its argument list + # and inserts each record, +push+ and +concat+ behave identically. def concat(*records) records = records.flatten if owner.new_record? diff --git a/activerecord/lib/active_record/associations/collection_proxy.rb b/activerecord/lib/active_record/associations/collection_proxy.rb index 4fbbc713e4..78993cee8a 100644 --- a/activerecord/lib/active_record/associations/collection_proxy.rb +++ b/activerecord/lib/active_record/associations/collection_proxy.rb @@ -366,34 +366,6 @@ module ActiveRecord @association.create!(attributes, &block) end - # Add one or more records to the collection by setting their foreign keys - # to the association's primary key. Since #<< flattens its argument list and - # inserts each record, +push+ and #concat behave identically. Returns +self+ - # so method calls may be chained. - # - # class Person < ActiveRecord::Base - # has_many :pets - # end - # - # person.pets.size # => 0 - # person.pets.concat(Pet.new(name: 'Fancy-Fancy')) - # person.pets.concat(Pet.new(name: 'Spook'), Pet.new(name: 'Choo-Choo')) - # person.pets.size # => 3 - # - # person.id # => 1 - # person.pets - # # => [ - # # #, - # # #, - # # # - # # ] - # - # person.pets.concat([Pet.new(name: 'Brain'), Pet.new(name: 'Benny')]) - # person.pets.size # => 5 - def concat(*records) - @association.concat(*records) - end - # Replaces this collection with +other_array+. This will perform a diff # and delete/add only records that have changed. # @@ -1033,8 +1005,9 @@ module ActiveRecord end # Adds one or more +records+ to the collection by setting their foreign keys - # to the association's primary key. Returns +self+, so several appends may be - # chained together. + # to the association's primary key. Since +<<+ flattens its argument list and + # inserts each record, +push+ and +concat+ behave identically. Returns +self+ + # so several appends may be chained together. # # class Person < ActiveRecord::Base # has_many :pets @@ -1057,6 +1030,7 @@ module ActiveRecord end alias_method :push, :<< alias_method :append, :<< + alias_method :concat, :<< def prepend(*args) raise NoMethodError, "prepend on association is not defined. Please use <<, push or append" diff --git a/activerecord/test/cases/associations/has_many_associations_test.rb b/activerecord/test/cases/associations/has_many_associations_test.rb index 4c9e4d0ad2..5fdc5a92fc 100644 --- a/activerecord/test/cases/associations/has_many_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_associations_test.rb @@ -996,9 +996,10 @@ class HasManyAssociationsTest < ActiveRecord::TestCase assert_predicate companies(:first_firm).clients_of_firm, :loaded? - companies(:first_firm).clients_of_firm.concat([Client.new("name" => "Natural Company"), Client.new("name" => "Apple")]) + result = companies(:first_firm).clients_of_firm.concat([Client.new("name" => "Natural Company"), Client.new("name" => "Apple")]) assert_equal 4, companies(:first_firm).clients_of_firm.size assert_equal 4, companies(:first_firm).clients_of_firm.reload.size + assert_equal companies(:first_firm).clients_of_firm, result end def test_transactions_when_adding_to_persisted diff --git a/activerecord/test/cases/associations/has_many_through_associations_test.rb b/activerecord/test/cases/associations/has_many_through_associations_test.rb index 0133beccec..0ac56c6168 100644 --- a/activerecord/test/cases/associations/has_many_through_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_through_associations_test.rb @@ -242,9 +242,10 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase def test_concat person = people(:david) post = posts(:thinking) - post.people.concat [person] + result = post.people.concat [person] assert_equal 1, post.people.size assert_equal 1, post.people.reload.size + assert_equal post.people, result end def test_associate_existing_record_twice_should_add_to_target_twice -- cgit v1.2.3