Merge pull request rails#28155 from lcreid/belongs_to

Fix "autosave: true" on belongs_to of join model causes invalid records to be saved

Author: kamipo

activerecord/CHANGELOG.md

@@ -1,3 +1,17 @@
+*   Fix circular `autosave: true` causes invalid records to be saved.
+
+    Prior to the fix, 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*
+
 *   Raise `ArgumentError` for invalid `:limit` and `:precision` like as other options.
 
     Before:

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.
+          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,
@@ -1492,6 +1495,19 @@ def check_pet!(added)
     end
   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

@@ -792,6 +792,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