From 018697dece967f3f2861a085e747ba14f06c507c Mon Sep 17 00:00:00 2001 From: "Dr.(USA) Joerg Schray" Date: Fri, 16 Mar 2012 12:26:24 +0100 Subject: Fix interactions between :before_add callbacks and nested attributes assignment Issue #1: :before_add callback is called when nested attributes assignment assigns to existing record if the association is not yet loaded Issue #2: Nested Attributes assignment does not affect the record in the association target when callback triggers loading of the association --- activerecord/CHANGELOG.md | 15 ++- .../lib/active_record/nested_attributes.rb | 19 ++- .../cases/nested_attributes_with_callbacks_test.rb | 145 +++++++++++++++++++++ 3 files changed, 168 insertions(+), 11 deletions(-) create mode 100644 activerecord/test/cases/nested_attributes_with_callbacks_test.rb (limited to 'activerecord') diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 3010f42cd6..6b3e5df1a7 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,5 +1,18 @@ +* Fix interactions between `:before_add` callbacks and nested attributes + assignment of `has_many` associations, when the association was not + yet loaded: + + - A `:before_add` callback was being called when a nested attributes + assignment assigned to an existing record. + + - Nested Attributes assignment did not affect the record in the + association target when a `:before_add` callback triggered the + loading of the association + + *Jörg Schray* + * Allow enable_extension migration method to be revertible. - + *Eric Tipton* * Type cast hstore values on write, so that the value is consistent diff --git a/activerecord/lib/active_record/nested_attributes.rb b/activerecord/lib/active_record/nested_attributes.rb index e53e8553ad..403ad98c3b 100644 --- a/activerecord/lib/active_record/nested_attributes.rb +++ b/activerecord/lib/active_record/nested_attributes.rb @@ -465,16 +465,15 @@ module ActiveRecord association.build(attributes.except(*UNASSIGNABLE_KEYS)) end elsif existing_record = existing_records.detect { |record| record.id.to_s == attributes['id'].to_s } - unless association.loaded? || call_reject_if(association_name, attributes) - # Make sure we are operating on the actual object which is in the association's - # proxy_target array (either by finding it, or adding it if not found) - target_record = association.target.detect { |record| record == existing_record } - - if target_record - existing_record = target_record - else - association.add_to_target(existing_record) - end + # Make sure we are operating on the actual object which is in the association's + # proxy_target array (either by finding it, or adding it if not found) + # Take into account that the proxy_target may have changed due to callbacks + target_record = association.target.detect { |record| record.id.to_s == attributes['id'].to_s } + if target_record + existing_record = target_record + else + #FIXME: there is no good way of adding the record without callback + association.target << existing_record end if !call_reject_if(association_name, attributes) diff --git a/activerecord/test/cases/nested_attributes_with_callbacks_test.rb b/activerecord/test/cases/nested_attributes_with_callbacks_test.rb new file mode 100644 index 0000000000..95989ffed4 --- /dev/null +++ b/activerecord/test/cases/nested_attributes_with_callbacks_test.rb @@ -0,0 +1,145 @@ +require "cases/helper" +require "models/pirate" +require "models/bird" + +class TestNestedAttributesWithCallbacksInterferingWithAssignment < ActiveRecord::TestCase + Pirate.has_many(:birds_with_interfering_callback, + :class_name => "Bird", + :before_add => proc { |p,b| + @@add_callback_called << b + p.birds_with_interfering_callback.to_a + }) + Pirate.has_many(:birds_with_callback, + :class_name => "Bird", + :before_add => proc { |p,b| @@add_callback_called << b }) + + Pirate.accepts_nested_attributes_for(:birds_with_interfering_callback, + :birds_with_callback, + :allow_destroy => true) + + def setup + @@add_callback_called = [] + @expect_callbacks_for = [] + @pirate_with_two_birds = Pirate.new.tap do |pirate| + pirate.catchphrase = "Don't call me!" + pirate.birds_attributes = [{:name => 'Bird1'},{:name => 'Bird2'}] + pirate.save! + end + @birds = @pirate_with_two_birds.birds.to_a + @birds_attributes = @birds.map do |bird| + bird.attributes.slice("id","name") + end + end + + def new_birds + @pirate_with_two_birds.birds_with_callback.to_a - @birds + end + + def new_bird_attributes + [{'name' => "New Bird"}] + end + + def bird2_deletion_attributes + [{'id' => @birds[1].id.to_s, "_destroy" => true}] + end + + def update_new_and_destroy_bird_attributes + [{'id' => @birds[0].id.to_s, 'name' => 'New Name'}, + {'name' => "New Bird"}, + {'id' => @birds[1].id.to_s, "_destroy" => true}] + end + + # Characterizing when :before_add callback is called + test ":before_add called for new bird when not loaded" do + assert_birds_with_callback_not_loaded + assert_callback_called_for_new_bird_assignment + end + + test ":before_add called for new bird when loaded" do + @pirate_with_two_birds.birds_with_callback.load_target + assert_callback_called_for_new_bird_assignment + end + + def assert_callback_called_for_new_bird_assignment + @pirate_with_two_birds.birds_with_callback_attributes = new_bird_attributes + assert_equal(1,new_birds.size) + assert_callback_called_for_new_birds + end + + test ":before_add not called for identical assignment when not loaded" do + assert_birds_with_callback_not_loaded + assert_callback_not_called_for_identical_assignment + end + + test ":before_add not called for identical assignment when loaded" do + @pirate_with_two_birds.birds_with_callback.load_target + assert_callback_not_called_for_identical_assignment + end + + def assert_callback_not_called_for_identical_assignment + @pirate_with_two_birds.birds_with_callback_attributes = @birds_attributes + assert_equal([],new_birds) + assert_callback_called_for_new_birds + end + + test ":before_add not called for destroy assignment when not loaded" do + assert_birds_with_callback_not_loaded + assert_callback_not_called_for_destroy_assignment + end + + test ":before_add not called for destroy assignment when loaded" do + @pirate_with_two_birds.birds_with_callback.load_target + assert_callback_not_called_for_destroy_assignment + end + + def assert_callback_not_called_for_destroy_assignment + @pirate_with_two_birds.birds_with_callback_attributes = + bird2_deletion_attributes + assert_callback_called_for_new_birds + end + + def assert_birds_with_callback_not_loaded + assert_equal(false,@pirate_with_two_birds.birds_with_callback.loaded?) + end + + def assert_callback_called_for_new_birds + assert_equal(new_birds,@@add_callback_called) + end + + # Ensuring that the records in the association target are updated, + # whether the association is loaded before or not + test "Assignment updates records in target when not loaded" do + assert_equal(false,@pirate_with_two_birds.birds_with_callback.loaded?) + assert_assignment_affects_records_in_target(:birds_with_callback) + end + + test "Assignment updates records in target when loaded" do + @pirate_with_two_birds.birds_with_callback.load_target + assert_assignment_affects_records_in_target(:birds_with_callback) + end + + test("Assignment updates records in target when not loaded" + + " and callback loads target") do + assert_equal(false, + @pirate_with_two_birds.birds_with_interfering_callback.loaded?) + assert_assignment_affects_records_in_target( + :birds_with_interfering_callback) + end + + test("Assignment updates records in target when loaded" + + " and callback loads target") do + @pirate_with_two_birds.birds_with_interfering_callback.load_target + assert_assignment_affects_records_in_target( + :birds_with_interfering_callback) + end + + def assert_assignment_affects_records_in_target(association_name) + @pirate_with_two_birds.send("#{association_name}_attributes=", + update_new_and_destroy_bird_attributes) + association = @pirate_with_two_birds.send(association_name) + birds_in_target = @birds.map { |b| association.detect { |b_in_t| b_in_t == b }} + assert_equal(true,birds_in_target[0].name_changed?,'First record not changed') + assert_equal(true,birds_in_target[1].marked_for_destruction?, + 'Second record not marked for destruction') + end +end -- cgit v1.2.3 From d35e900c0046126d901ae9a78ce33e28f6c97644 Mon Sep 17 00:00:00 2001 From: Ben Woosley Date: Sat, 20 Jul 2013 17:23:45 -0700 Subject: Restore the use of `#add_to_target` for nested attribute updates on existing records, and don't bother updating the association if the update is going to be rejected anyway. This requires adding a `skip_callbacks` argument to `#add_to_target` so that we don't call the callbacks multiple times in this case, which is functionally an application of existing association data, rather than an addition of a new record to the association. --- activerecord/CHANGELOG.md | 6 ++++++ .../associations/collection_association.rb | 8 ++++---- activerecord/lib/active_record/nested_attributes.rb | 21 ++++++++++----------- 3 files changed, 20 insertions(+), 15 deletions(-) (limited to 'activerecord') diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 6b3e5df1a7..e8ec99511e 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,9 @@ +* `add_to_target` now accepts a second optional `skip_callbacks` argument + + If truthy, it will skip the :before_add and :after_add callbacks. + + *Ben Woosley* + * Fix interactions between `:before_add` callbacks and nested attributes assignment of `has_many` associations, when the association was not yet loaded: diff --git a/activerecord/lib/active_record/associations/collection_association.rb b/activerecord/lib/active_record/associations/collection_association.rb index 8ce02afef8..228c500f0a 100644 --- a/activerecord/lib/active_record/associations/collection_association.rb +++ b/activerecord/lib/active_record/associations/collection_association.rb @@ -290,7 +290,7 @@ module ActiveRecord # Returns true if the collection is empty. # - # If the collection has been loaded + # If the collection has been loaded # it is equivalent to collection.size.zero?. If the # collection has not been loaded, it is equivalent to # collection.exists?. If the collection has not already been @@ -366,8 +366,8 @@ module ActiveRecord target end - def add_to_target(record) - callback(:before_add, record) + def add_to_target(record, skip_callbacks = false) + callback(:before_add, record) unless skip_callbacks yield(record) if block_given? if association_scope.distinct_value && index = @target.index(record) @@ -376,7 +376,7 @@ module ActiveRecord @target << record end - callback(:after_add, record) + callback(:after_add, record) unless skip_callbacks set_inverse_instance(record) record diff --git a/activerecord/lib/active_record/nested_attributes.rb b/activerecord/lib/active_record/nested_attributes.rb index 403ad98c3b..df28451bb7 100644 --- a/activerecord/lib/active_record/nested_attributes.rb +++ b/activerecord/lib/active_record/nested_attributes.rb @@ -465,18 +465,17 @@ module ActiveRecord association.build(attributes.except(*UNASSIGNABLE_KEYS)) end elsif existing_record = existing_records.detect { |record| record.id.to_s == attributes['id'].to_s } - # Make sure we are operating on the actual object which is in the association's - # proxy_target array (either by finding it, or adding it if not found) - # Take into account that the proxy_target may have changed due to callbacks - target_record = association.target.detect { |record| record.id.to_s == attributes['id'].to_s } - if target_record - existing_record = target_record - else - #FIXME: there is no good way of adding the record without callback - association.target << existing_record - end + unless call_reject_if(association_name, attributes) + # Make sure we are operating on the actual object which is in the association's + # proxy_target array (either by finding it, or adding it if not found) + # Take into account that the proxy_target may have changed due to callbacks + target_record = association.target.detect { |record| record.id.to_s == attributes['id'].to_s } + if target_record + existing_record = target_record + else + association.add_to_target(existing_record, :skip_callbacks) + end - if !call_reject_if(association_name, attributes) assign_to_or_mark_for_destruction(existing_record, attributes, options[:allow_destroy]) end else -- cgit v1.2.3 From cc368c3ab39bc376181d883c31877d9729fe3423 Mon Sep 17 00:00:00 2001 From: Ben Woosley Date: Sat, 20 Jul 2013 17:56:06 -0700 Subject: Refactor NestedAttributesWithCallbacksTest for clarity 1) Use `assert` and `refute` where possible. 2) Separately include the setup, subject, and assertions in each test - don't hide the tested call in an assertion method. 3) Name things based on their role rather than incidental facts about them - e.g. `@bird[1]` -> `bird_to_destroy`. `bird2_deletion_attributes` -> `destroy_bird_attributes`. 4) Use more succinct naming where possible - e.g. `birds_with_callback` -> `birds_with_add`, `@pirate_with_two_birds` -> `@pirate` --- .../cases/nested_attributes_with_callbacks_test.rb | 133 ++++++++++----------- 1 file changed, 66 insertions(+), 67 deletions(-) (limited to 'activerecord') diff --git a/activerecord/test/cases/nested_attributes_with_callbacks_test.rb b/activerecord/test/cases/nested_attributes_with_callbacks_test.rb index 95989ffed4..357b6b4545 100644 --- a/activerecord/test/cases/nested_attributes_with_callbacks_test.rb +++ b/activerecord/test/cases/nested_attributes_with_callbacks_test.rb @@ -2,144 +2,143 @@ require "cases/helper" require "models/pirate" require "models/bird" -class TestNestedAttributesWithCallbacksInterferingWithAssignment < ActiveRecord::TestCase - Pirate.has_many(:birds_with_interfering_callback, +class NestedAttributesWithCallbacksTest < ActiveRecord::TestCase + Pirate.has_many(:birds_with_add_load, :class_name => "Bird", :before_add => proc { |p,b| @@add_callback_called << b - p.birds_with_interfering_callback.to_a + p.birds_with_add_load.to_a }) - Pirate.has_many(:birds_with_callback, + Pirate.has_many(:birds_with_add, :class_name => "Bird", :before_add => proc { |p,b| @@add_callback_called << b }) - Pirate.accepts_nested_attributes_for(:birds_with_interfering_callback, - :birds_with_callback, + Pirate.accepts_nested_attributes_for(:birds_with_add_load, + :birds_with_add, :allow_destroy => true) def setup @@add_callback_called = [] - @expect_callbacks_for = [] - @pirate_with_two_birds = Pirate.new.tap do |pirate| + @pirate = Pirate.new.tap do |pirate| pirate.catchphrase = "Don't call me!" pirate.birds_attributes = [{:name => 'Bird1'},{:name => 'Bird2'}] pirate.save! end - @birds = @pirate_with_two_birds.birds.to_a - @birds_attributes = @birds.map do |bird| + @birds = @pirate.birds.to_a + end + + def bird_to_update + @birds[0] + end + + def bird_to_destroy + @birds[1] + end + + def existing_birds_attributes + @birds.map do |bird| bird.attributes.slice("id","name") end end def new_birds - @pirate_with_two_birds.birds_with_callback.to_a - @birds + @pirate.birds_with_add.to_a - @birds end def new_bird_attributes [{'name' => "New Bird"}] end - def bird2_deletion_attributes - [{'id' => @birds[1].id.to_s, "_destroy" => true}] + def destroy_bird_attributes + [{'id' => bird_to_destroy.id.to_s, "_destroy" => true}] end def update_new_and_destroy_bird_attributes [{'id' => @birds[0].id.to_s, 'name' => 'New Name'}, {'name' => "New Bird"}, - {'id' => @birds[1].id.to_s, "_destroy" => true}] + {'id' => bird_to_destroy.id.to_s, "_destroy" => true}] end # Characterizing when :before_add callback is called test ":before_add called for new bird when not loaded" do - assert_birds_with_callback_not_loaded - assert_callback_called_for_new_bird_assignment + refute @pirate.birds_with_add.loaded? + @pirate.birds_with_add_attributes = new_bird_attributes + assert_new_bird_with_callback_called end test ":before_add called for new bird when loaded" do - @pirate_with_two_birds.birds_with_callback.load_target - assert_callback_called_for_new_bird_assignment + @pirate.birds_with_add.load_target + @pirate.birds_with_add_attributes = new_bird_attributes + assert_new_bird_with_callback_called end - def assert_callback_called_for_new_bird_assignment - @pirate_with_two_birds.birds_with_callback_attributes = new_bird_attributes - assert_equal(1,new_birds.size) - assert_callback_called_for_new_birds + def assert_new_bird_with_callback_called + assert_equal(1, new_birds.size) + assert_equal(new_birds, @@add_callback_called) end test ":before_add not called for identical assignment when not loaded" do - assert_birds_with_callback_not_loaded - assert_callback_not_called_for_identical_assignment + refute @pirate.birds_with_add.loaded? + @pirate.birds_with_add_attributes = existing_birds_attributes + assert_callbacks_not_called end test ":before_add not called for identical assignment when loaded" do - @pirate_with_two_birds.birds_with_callback.load_target - assert_callback_not_called_for_identical_assignment - end - - def assert_callback_not_called_for_identical_assignment - @pirate_with_two_birds.birds_with_callback_attributes = @birds_attributes - assert_equal([],new_birds) - assert_callback_called_for_new_birds + @pirate.birds_with_add.load_target + @pirate.birds_with_add_attributes = existing_birds_attributes + assert_callbacks_not_called end test ":before_add not called for destroy assignment when not loaded" do - assert_birds_with_callback_not_loaded - assert_callback_not_called_for_destroy_assignment - end - - test ":before_add not called for destroy assignment when loaded" do - @pirate_with_two_birds.birds_with_callback.load_target - assert_callback_not_called_for_destroy_assignment - end - - def assert_callback_not_called_for_destroy_assignment - @pirate_with_two_birds.birds_with_callback_attributes = - bird2_deletion_attributes - assert_callback_called_for_new_birds + refute @pirate.birds_with_add.loaded? + @pirate.birds_with_add_attributes = destroy_bird_attributes + assert_callbacks_not_called end - def assert_birds_with_callback_not_loaded - assert_equal(false,@pirate_with_two_birds.birds_with_callback.loaded?) + test ":before_add not called for deletion assignment when loaded" do + @pirate.birds_with_add.load_target + @pirate.birds_with_add_attributes = destroy_bird_attributes + assert_callbacks_not_called end - def assert_callback_called_for_new_birds - assert_equal(new_birds,@@add_callback_called) + def assert_callbacks_not_called + assert_empty new_birds + assert_empty @@add_callback_called end # Ensuring that the records in the association target are updated, # whether the association is loaded before or not test "Assignment updates records in target when not loaded" do - assert_equal(false,@pirate_with_two_birds.birds_with_callback.loaded?) - assert_assignment_affects_records_in_target(:birds_with_callback) + refute @pirate.birds_with_add.loaded? + @pirate.birds_with_add_attributes = update_new_and_destroy_bird_attributes + assert_assignment_affects_records_in_target(:birds_with_add) end test "Assignment updates records in target when loaded" do - @pirate_with_two_birds.birds_with_callback.load_target - assert_assignment_affects_records_in_target(:birds_with_callback) + @pirate.birds_with_add.load_target + @pirate.birds_with_add_attributes = update_new_and_destroy_bird_attributes + assert_assignment_affects_records_in_target(:birds_with_add) end test("Assignment updates records in target when not loaded" + " and callback loads target") do - assert_equal(false, - @pirate_with_two_birds.birds_with_interfering_callback.loaded?) - assert_assignment_affects_records_in_target( - :birds_with_interfering_callback) + refute @pirate.birds_with_add_load.loaded? + @pirate.birds_with_add_load_attributes = update_new_and_destroy_bird_attributes + assert_assignment_affects_records_in_target(:birds_with_add_load) end test("Assignment updates records in target when loaded" + " and callback loads target") do - @pirate_with_two_birds.birds_with_interfering_callback.load_target - assert_assignment_affects_records_in_target( - :birds_with_interfering_callback) + @pirate.birds_with_add_load.load_target + @pirate.birds_with_add_load_attributes = update_new_and_destroy_bird_attributes + assert_assignment_affects_records_in_target(:birds_with_add_load) end def assert_assignment_affects_records_in_target(association_name) - @pirate_with_two_birds.send("#{association_name}_attributes=", - update_new_and_destroy_bird_attributes) - association = @pirate_with_two_birds.send(association_name) - birds_in_target = @birds.map { |b| association.detect { |b_in_t| b_in_t == b }} - assert_equal(true,birds_in_target[0].name_changed?,'First record not changed') - assert_equal(true,birds_in_target[1].marked_for_destruction?, - 'Second record not marked for destruction') + association = @pirate.send(association_name) + assert association.detect {|b| b == bird_to_update }.name_changed?, + 'Update record not updated' + assert association.detect {|b| b == bird_to_destroy }.marked_for_destruction?, + 'Destroy record not marked for destruction' end end -- cgit v1.2.3