Merge pull request #15579 from sgrif/sg-through-associations

Through associations should set both parent ids on join models

activerecord/CHANGELOG.md

+*   Ensure both parent IDs are set on join records when both sides of a
+    through association are new.
+
+    *Sean Griffin*
+
 *   `ActiveRecord::Dirty` now detects in-place changes to mutable values.
     Serialized attributes on ActiveRecord models will no longer save when
     unchanged. Fixes #8328.

activerecord/lib/active_record/associations/has_many_through_association.rb

@@ -180,7 +180,11 @@ def delete_records(records, method)
         def through_records_for(record)
           attributes = construct_join_attributes(record)
           candidates = Array.wrap(through_association.target)
-          candidates.find_all { |c| c.attributes.slice(*attributes.keys) == attributes }
+          candidates.find_all do |c|
+            attributes.all? do |key, value|
+              c.public_send(key) == value
+            end
+          end
         end
 
         def delete_through_records(records)

activerecord/lib/active_record/associations/through_association.rb

@@ -41,12 +41,16 @@ def target_scope
         def construct_join_attributes(*records)
           ensure_mutable
 
-          join_attributes = {
-            source_reflection.foreign_key =>
-              records.map { |record|
-                record.send(source_reflection.association_primary_key(reflection.klass))
-              }
-          }
+          if source_reflection.association_primary_key(reflection.klass) == reflection.klass.primary_key
+            join_attributes = { source_reflection.name => records }
+          else
+            join_attributes = {
+              source_reflection.foreign_key =>
+                records.map { |record|
+                  record.send(source_reflection.association_primary_key(reflection.klass))
+                }
+            }
+          end
 
           if options[:source_type]
             join_attributes[source_reflection.foreign_type] =

activerecord/test/cases/associations/has_many_through_associations_test.rb

@@ -330,6 +330,19 @@ def test_build_then_save_with_has_one_inverse
     assert post.single_people.include?(person)
   end
 
+  def test_both_parent_ids_set_when_saving_new
+    post = Post.new(title: 'Hello', body: 'world')
+    person = Person.new(first_name: 'Sean')
+
+    post.people = [person]
+    post.save
+
+    assert post.id
+    assert person.id
+    assert_equal post.id, post.readers.first.post_id
+    assert_equal person.id, post.readers.first.person_id
+  end
+
   def test_delete_association
     assert_queries(2){posts(:welcome);people(:michael); }
 

activerecord/test/cases/associations/has_one_through_associations_test.rb

@@ -45,6 +45,20 @@ def test_creating_association_builds_through_record_for_new
     assert_equal clubs(:moustache_club), new_member.club
   end
 
+  def test_creating_association_sets_both_parent_ids_for_new
+    member = Member.new(name: 'Sean Griffin')
+    club = Club.new(name: 'Da Club')
+
+    member.club = club
+
+    member.save!
+
+    assert member.id
+    assert club.id
+    assert_equal member.id, member.current_membership.member_id
+    assert_equal club.id, member.current_membership.club_id
+  end
+
   def test_replace_target_record
     new_club = Club.create(:name => "Marx Bros")
     @member.club = new_club