TEST_CODEOWNERS/rails
3
0
Fork 0
Code Issues Pull Requests 1 Actions Packages Projects Releases Wiki Activity

Merge pull request #49847 from joshuay03/duplicate-callbacks-when-child-autosaves-parent

Browse Source 
[Fix #48688] Duplicate callback execution when child autosaves parent with `has_one` and `belongs_to`
This commit is contained in:
Jean Boussier 2024-07-08 19:00:47 +02:00 committed by GitHub
commit 16f1222753
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
4 changed files with 133 additions and 42 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

9
activerecord/CHANGELOG.md
View File

@ -1,3 +1,12 @@
* Fix duplicate callback execution when child autosaves parent with `has_one` and `belongs_to`.
Before, persisting a new child record with a new associated parent record would run `before_validation`,
`after_validation`, `before_save` and `after_save` callbacks twice.
Now, these callbacks are only executed once as expected.
*Joshua Young*
* `ActiveRecord::Encryption::Encryptor` now supports a `:compressor` option to customize the compression algorithm used.
```ruby

56
activerecord/lib/active_record/autosave_association.rb
View File

@ -317,7 +317,12 @@ def nested_records_changed_for_autosave?
def validate_single_association(reflection)
association = association_instance_get(reflection.name)
record = association && association.reader
association_valid?(association, record) if record && (record.changed_for_autosave? || custom_validation_context?)
return unless record && (record.changed_for_autosave? || custom_validation_context?)
inverse_belongs_to_association = inverse_belongs_to_association_for(reflection, record)
return if inverse_belongs_to_association && inverse_belongs_to_association.updated?
association_valid?(association, record)
end
# Validate the associated records if <tt>:validate</tt> or
@ -429,36 +434,44 @@ def save_collection_association(reflection)
def save_has_one_association(reflection)
association = association_instance_get(reflection.name)
record = association && association.load_target
return unless record && !record.destroyed?
if record && !record.destroyed?
autosave = reflection.options[:autosave]
inverse_belongs_to_association = inverse_belongs_to_association_for(reflection, record)
return if inverse_belongs_to_association && inverse_belongs_to_association.updated?
if autosave && record.marked_for_destruction?
record.destroy
elsif autosave != false
primary_key = Array(compute_primary_key(reflection, self)).map(&:to_s)
primary_key_value = primary_key.map { |key| _read_attribute(key) }
autosave = reflection.options[:autosave]
if (autosave && record.changed_for_autosave?) || _record_changed?(reflection, record, primary_key_value)
unless reflection.through_reflection
foreign_key = Array(reflection.foreign_key)
primary_key_foreign_key_pairs = primary_key.zip(foreign_key)
if autosave && record.marked_for_destruction?
record.destroy
elsif autosave != false
primary_key = Array(compute_primary_key(reflection, self)).map(&:to_s)
primary_key_value = primary_key.map { |key| _read_attribute(key) }
primary_key_foreign_key_pairs.each do |primary_key, foreign_key|
association_id = _read_attribute(primary_key)
record[foreign_key] = association_id unless record[foreign_key] == association_id
end
association.set_inverse_instance(record)
if (autosave && record.changed_for_autosave?) || _record_changed?(reflection, record, primary_key_value)
unless reflection.through_reflection
foreign_key = Array(reflection.foreign_key)
primary_key_foreign_key_pairs = primary_key.zip(foreign_key)
primary_key_foreign_key_pairs.each do |primary_key, foreign_key|
association_id = _read_attribute(primary_key)
record[foreign_key] = association_id unless record[foreign_key] == association_id
end
saved = record.save(validate: !autosave)
raise ActiveRecord::Rollback if !saved && autosave
saved
association.set_inverse_instance(record)
end
saved = record.save(validate: !autosave)
raise ActiveRecord::Rollback if !saved && autosave
saved
end
end
end
def inverse_belongs_to_association_for(reflection, record)
reflection.inverse_of &&
reflection.inverse_of.belongs_to? &&
record.association(reflection.inverse_of.name)
end
# If the record is new or it has changed, returns true.
def _record_changed?(reflection, record, key)
record.new_record? ||
@ -480,7 +493,6 @@ def inverse_polymorphic_association_changed?(reflection, record)
return false unless reflection.inverse_of&.polymorphic?
class_name = record._read_attribute(reflection.inverse_of.foreign_type)
reflection.active_record != record.class.polymorphic_class_for(class_name)
end

20
activerecord/test/cases/autosave_association_test.rb
View File

@ -304,6 +304,26 @@ def test_callbacks_firing_order_on_save
assert_equal [false, false, false, false], eye.after_save_callbacks_stack
end
def test_callbacks_on_child_when_parent_autosaves_child
eye = Eye.create(iris: Iris.new)
assert_equal 1, eye.iris.before_validation_callbacks_counter
assert_equal 1, eye.iris.before_create_callbacks_counter
assert_equal 1, eye.iris.before_save_callbacks_counter
assert_equal 1, eye.iris.after_validation_callbacks_counter
assert_equal 1, eye.iris.after_create_callbacks_counter
assert_equal 1, eye.iris.after_save_callbacks_counter
end
def test_callbacks_on_child_when_child_autosaves_parent
iris = Iris.create(eye: Eye.new)
assert_equal 1, iris.before_validation_callbacks_counter
assert_equal 1, iris.before_create_callbacks_counter
assert_equal 1, iris.before_save_callbacks_counter
assert_equal 1, iris.after_validation_callbacks_counter
assert_equal 1, iris.after_create_callbacks_counter
assert_equal 1, iris.after_save_callbacks_counter
end
def test_foreign_key_attribute_is_not_set_unless_changed
eye = Eye.create!(iris_with_read_only_foreign_key_attributes: { color: "honey" })
assert_nothing_raised do

90
activerecord/test/models/eye.rb
View File

@ -19,35 +19,85 @@ class Eye < ActiveRecord::Base
after_update :trace_after_update2, if: :iris
after_save :trace_after_save2, if: :iris
def trace_after_create
(@after_create_callbacks_stack ||= []) << !iris.persisted?
end
alias trace_after_create2 trace_after_create
private
def trace_after_create
(@after_create_callbacks_stack ||= []) << !iris.persisted?
end
alias trace_after_create2 trace_after_create
def trace_after_update
(@after_update_callbacks_stack ||= []) << iris.has_changes_to_save?
end
alias trace_after_update2 trace_after_update
def trace_after_update
(@after_update_callbacks_stack ||= []) << iris.has_changes_to_save?
end
alias trace_after_update2 trace_after_update
def trace_after_save
(@after_save_callbacks_stack ||= []) << iris.has_changes_to_save?
end
alias trace_after_save2 trace_after_save
def trace_after_save
(@after_save_callbacks_stack ||= []) << iris.has_changes_to_save?
end
alias trace_after_save2 trace_after_save
has_one :iris_with_read_only_foreign_key, class_name: "IrisWithReadOnlyForeignKey", foreign_key: :eye_id
accepts_nested_attributes_for :iris_with_read_only_foreign_key
public
has_one :iris_with_read_only_foreign_key, class_name: "IrisWithReadOnlyForeignKey", foreign_key: :eye_id
accepts_nested_attributes_for :iris_with_read_only_foreign_key
before_save :set_iris_with_read_only_foreign_key_color_to_blue, if: -> {
iris_with_read_only_foreign_key && @override_iris_with_read_only_foreign_key_color
}
before_save :set_iris_with_read_only_foreign_key_color_to_blue, if: -> {
iris_with_read_only_foreign_key && @override_iris_with_read_only_foreign_key_color
}
def set_iris_with_read_only_foreign_key_color_to_blue
iris_with_read_only_foreign_key.color = "blue"
end
private
def set_iris_with_read_only_foreign_key_color_to_blue
iris_with_read_only_foreign_key.color = "blue"
end
end
class Iris < ActiveRecord::Base
attr_reader :before_validation_callbacks_counter
attr_reader :before_create_callbacks_counter
attr_reader :before_save_callbacks_counter
attr_reader :after_validation_callbacks_counter
attr_reader :after_create_callbacks_counter
attr_reader :after_save_callbacks_counter
belongs_to :eye
before_validation :update_before_validation_counter
before_create :update_before_create_counter
before_save :update_before_save_counter
after_validation :update_after_validation_counter
after_create :update_after_create_counter
after_save :update_after_save_counter
private
def update_before_validation_counter
@before_validation_callbacks_counter ||= 0
@before_validation_callbacks_counter += 1
end
def update_before_create_counter
@before_create_callbacks_counter ||= 0
@before_create_callbacks_counter += 1
end
def update_before_save_counter
@before_save_callbacks_counter ||= 0
@before_save_callbacks_counter += 1
end
def update_after_validation_counter
@after_validation_callbacks_counter ||= 0
@after_validation_callbacks_counter += 1
end
def update_after_create_counter
@after_create_callbacks_counter ||= 0
@after_create_callbacks_counter += 1
end
def update_after_save_counter
@after_save_callbacks_counter ||= 0
@after_save_callbacks_counter += 1
end
end
class IrisWithReadOnlyForeignKey < Iris