diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 456f569718..b74ad35a92 100644 --- a/activerecord/CHANGELOG.md +++ b/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 diff --git a/activerecord/lib/active_record/autosave_association.rb b/activerecord/lib/active_record/autosave_association.rb index a405f05e0b..78acc06eae 100644 --- a/activerecord/lib/active_record/autosave_association.rb +++ b/activerecord/lib/active_record/autosave_association.rb @@ -382,10 +382,14 @@ module ActiveRecord 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 @@ module ActiveRecord 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? 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 d5573b6d02..97e83441a2 100644 --- a/activerecord/test/cases/associations/has_many_through_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_through_associations_test.rb @@ -33,6 +33,9 @@ require "models/organization" 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 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase 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 } } diff --git a/activerecord/test/models/section.rb b/activerecord/test/models/section.rb new file mode 100644 index 0000000000..f8b4cc7936 --- /dev/null +++ b/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 diff --git a/activerecord/test/models/seminar.rb b/activerecord/test/models/seminar.rb new file mode 100644 index 0000000000..c18aa86433 --- /dev/null +++ b/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 diff --git a/activerecord/test/models/session.rb b/activerecord/test/models/session.rb new file mode 100644 index 0000000000..db66b5297e --- /dev/null +++ b/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 diff --git a/activerecord/test/schema/schema.rb b/activerecord/test/schema/schema.rb index 266e55f682..02520fc0cc 100644 --- a/activerecord/test/schema/schema.rb +++ b/activerecord/test/schema/schema.rb @@ -772,6 +772,24 @@ ActiveRecord::Schema.define do 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