Fix circular autosave: true

Use a variable local to the `save_collection_association` method in
`activerecord/lib/active_record/autosave_association.rb`, instead of an
instance variable.

Prior to this PR, when there was a circular series of `autosave: true`
associations, the callback for a `has_many` association was run while
another instance of the same callback on the same association hadn't
finished running. When control returned to the first instance of the
callback, the instance variable had changed, and subsequent associated
records weren't saved correctly. Specifically, the ID field for the
`belongs_to` corresponding to the `has_many` was `nil`.

Remove unnecessary test and comments.

Fixes rails#28080.

Author: lcreid

activerecord/CHANGELOG.md

@@ -1,3 +1,20 @@
+*   Fix circular `autosave: true`
+
+    Use a variable local to the `save_collection_association` method in
+    `activerecord/lib/active_record/autosave_association.rb`, instead of an
+    instance variable.
+
+    Prior to this PR, when there was a circular series of `autosave: true`
+    associations, the callback for a `has_many` association was run while
+    another instance of the same callback on the same association hadn't
+    finished running. When control returned to the first instance of the
+    callback, the instance variable had changed, and subsequent associated
+    records weren't saved correctly. Specifically, the ID field for the
+    `belongs_to` corresponding to the `has_many` was `nil`.
+
+    Fixes #28080.
+
+    *Larry Reid*
 *   Don't impose primary key order if limit() has already been supplied.
 
     Fixes #23607

activerecord/lib/active_record/autosave_association.rb

@@ -382,10 +382,14 @@ def save_collection_association(reflection)
         if association = association_instance_get(reflection.name)
           autosave = reflection.options[:autosave]
 
+          # By saving the instance variable in a local variable, we make the
+          # whole callback re-entrant, fixing issue #28080
+          new_record_before_save = @new_record_before_save
+
           # reconstruct the scope now that we know the owner's id
           association.reset_scope
 
-          if records = associated_records_to_validate_or_save(association, @new_record_before_save, autosave)
+          if records = associated_records_to_validate_or_save(association, new_record_before_save, autosave)
             if autosave
               records_to_destroy = records.select(&:marked_for_destruction?)
               records_to_destroy.each { |record| association.destroy(record) }
@@ -397,7 +401,7 @@ def save_collection_association(reflection)
 
               saved = true
 
-              if autosave != false && (@new_record_before_save || record.new_record?)
+              if autosave != false && (new_record_before_save || record.new_record?)
                 if autosave
                   saved = association.insert_record(record, false)
                 elsif !reflection.nested?

activerecord/test/cases/associations/has_many_through_associations_test.rb

@@ -33,6 +33,9 @@
 require "models/user"
 require "models/family"
 require "models/family_tree"
+require "models/section"
+require "models/seminar"
+require "models/session"
 
 class HasManyThroughAssociationsTest < ActiveRecord::TestCase
   fixtures :posts, :readers, :people, :comments, :authors, :categories, :taggings, :tags,
@@ -1403,6 +1406,19 @@ def self.name; "PostWithNestedHasManyThrough"; end
     assert_equal [subscription2], post.subscriptions.to_a
   end
 
+  def test_circular_autosave_association_correctly_saves_multiple_records
+    cs180 = Seminar.new(name: "CS180")
+    fall = Session.new(name: "Fall")
+    sections = [
+      cs180.sections.build(short_name: "A"),
+      cs180.sections.build(short_name: "B"),
+    ]
+    fall.sections << sections
+    fall.save!
+    fall.reload
+    assert_equal sections, fall.sections.sort_by(&:id)
+  end
+
   private
     def make_model(name)
       Class.new(ActiveRecord::Base) { define_singleton_method(:name) { name } }

activerecord/test/models/section.rb

@@ -0,0 +1,6 @@
+# frozen_string_literal: true
+
+class Section < ActiveRecord::Base
+  belongs_to :session, inverse_of: :sections, autosave: true
+  belongs_to :seminar, inverse_of: :sections, autosave: true
+end

activerecord/test/models/seminar.rb

@@ -0,0 +1,6 @@
+# frozen_string_literal: true
+
+class Seminar < ActiveRecord::Base
+  has_many :sections, inverse_of: :seminar, autosave: true, dependent: :destroy
+  has_many :sessions, through: :sections
+end

activerecord/test/models/session.rb

@@ -0,0 +1,6 @@
+# frozen_string_literal: true
+
+class Session < ActiveRecord::Base
+  has_many :sections, inverse_of: :session, autosave: true, dependent: :destroy
+  has_many :seminars, through: :sections
+end

activerecord/test/schema/schema.rb

@@ -772,6 +772,24 @@
     t.integer :lock_version, default: 0
   end
 
+  disable_referential_integrity do
+    create_table :seminars, force: :cascade do |t|
+      t.string :name
+    end
+
+    create_table :sessions, force: :cascade do |t|
+      t.date :start_date
+      t.date :end_date
+      t.string :name
+    end
+
+    create_table :sections, force: :cascade do |t|
+      t.string :short_name
+      t.belongs_to :session, foreign_key: true
+      t.belongs_to :seminar, foreign_key: true
+    end
+  end
+
   create_table :shape_expressions, force: true do |t|
     t.string  :paint_type
     t.integer :paint_id