From 332e7601a98ebff6a7494a556c7fe97c5691f085 Mon Sep 17 00:00:00 2001 From: Larry Reid Date: Mon, 16 Jul 2018 10:44:55 -0700 Subject: 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 #28080. --- activerecord/CHANGELOG.md | 17 +++++++++++++++++ activerecord/lib/active_record/autosave_association.rb | 8 ++++++-- .../associations/has_many_through_associations_test.rb | 16 ++++++++++++++++ activerecord/test/models/section.rb | 6 ++++++ activerecord/test/models/seminar.rb | 6 ++++++ activerecord/test/models/session.rb | 6 ++++++ activerecord/test/schema/schema.rb | 18 ++++++++++++++++++ 7 files changed, 75 insertions(+), 2 deletions(-) create mode 100644 activerecord/test/models/section.rb create mode 100644 activerecord/test/models/seminar.rb create mode 100644 activerecord/test/models/session.rb (limited to 'activerecord') 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 -- cgit v1.2.3