mirror of
https://github.com/rails/rails.git
synced 2022-11-09 12:12:34 -05:00
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.
This commit is contained in:
parent
bd139a5940
commit
332e7601a9
7 changed files with 75 additions and 2 deletions
|
@ -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
|
||||
|
|
|
@ -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?
|
||||
|
|
|
@ -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 } }
|
||||
|
|
6
activerecord/test/models/section.rb
Normal file
6
activerecord/test/models/section.rb
Normal file
|
@ -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
|
6
activerecord/test/models/seminar.rb
Normal file
6
activerecord/test/models/seminar.rb
Normal file
|
@ -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
|
6
activerecord/test/models/session.rb
Normal file
6
activerecord/test/models/session.rb
Normal file
|
@ -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
|
|
@ -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
|
||||
|
|
Loading…
Reference in a new issue