Make sure to not add autosave callbacks multiple times. [#3575 state:resolved]

This makes sure that, in a HABTM association, only one join record is craeted.

4 files changed, 65 insertions, 17 deletions

diff --git a/activerecord/lib/active_record/autosave_association.rb b/activerecord/lib/active_record/autosave_association.rb
index c0d8904bc8..44c668b619 100644
--- a/activerecord/lib/active_record/autosave_association.rb
+++ b/activerecord/lib/active_record/autosave_association.rb
@@ -155,6 +155,13 @@ module ActiveRecord
 
       # Adds a validate and save callback for the association as specified by
       # the +reflection+.
+      #
+      # For performance reasons, we don't check whether to validate at runtime,
+      # but instead only define the method and callback when needed. However,
+      # this can change, for instance, when using nested attributes. Since we
+      # don't want the callbacks to get defined multiple times, there are
+      # guards that check if the save or validation methods have already been
+      # defined before actually defining them.
       def add_autosave_association_callbacks(reflection)
         save_method = "autosave_associated_records_for_#{reflection.name}"
         validation_method = "validate_associated_records_for_#{reflection.name}"
@@ -162,28 +169,33 @@ module ActiveRecord
 
         case reflection.macro
         when :has_many, :has_and_belongs_to_many
-          before_save :before_save_collection_association
+          unless method_defined?(save_method)
+            before_save :before_save_collection_association
 
-          define_method(save_method) { save_collection_association(reflection) }
-          # Doesn't use after_save as that would save associations added in after_create/after_update twice
-          after_create save_method
-          after_update save_method
+            define_method(save_method) { save_collection_association(reflection) }
+            # Doesn't use after_save as that would save associations added in after_create/after_update twice
+            after_create save_method
+            after_update save_method
+          end
 
-          if force_validation || (reflection.macro == :has_many && reflection.options[:validate] != false)
+          if !method_defined?(validation_method) &&
+              (force_validation || (reflection.macro == :has_many && reflection.options[:validate] != false))
             define_method(validation_method) { validate_collection_association(reflection) }
             validate validation_method
           end
         else
-          case reflection.macro
-          when :has_one
-            define_method(save_method) { save_has_one_association(reflection) }
-            after_save save_method
-          when :belongs_to
-            define_method(save_method) { save_belongs_to_association(reflection) }
-            before_save save_method
+          unless method_defined?(save_method)
+            case reflection.macro
+            when :has_one
+              define_method(save_method) { save_has_one_association(reflection) }
+              after_save save_method
+            when :belongs_to
+              define_method(save_method) { save_belongs_to_association(reflection) }
+              before_save save_method
+            end
           end
 
-          if force_validation
+          if !method_defined?(validation_method) && force_validation
             define_method(validation_method) { validate_single_association(reflection) }
             validate validation_method
           end
diff --git a/activerecord/lib/active_record/nested_attributes.rb b/activerecord/lib/active_record/nested_attributes.rb
index ca3110a374..386f0e7ca7 100644
--- a/activerecord/lib/active_record/nested_attributes.rb
+++ b/activerecord/lib/active_record/nested_attributes.rb
@@ -235,7 +235,7 @@ module ActiveRecord
             end
 
             reflection.options[:autosave] = true
-
+            add_autosave_association_callbacks(reflection)
             self.nested_attributes_options[association_name.to_sym] = options
 
             if options[:reject_if] == :all_blank
@@ -250,8 +250,6 @@ module ActiveRecord
                 assign_nested_attributes_for_#{type}_association(:#{association_name}, attributes)
               end
             }, __FILE__, __LINE__
-
-            add_autosave_association_callbacks(reflection)
           else
             raise ArgumentError, "No association found for name `#{association_name}'. Has it been defined yet?"
           end
diff --git a/activerecord/test/cases/autosave_association_test.rb b/activerecord/test/cases/autosave_association_test.rb
index 9164701601..803e5b25b1 100644
--- a/activerecord/test/cases/autosave_association_test.rb
+++ b/activerecord/test/cases/autosave_association_test.rb
@@ -31,11 +31,40 @@ class TestAutosaveAssociationsInGeneral < ActiveRecord::TestCase
     assert base.valid_keys_for_has_and_belongs_to_many_association.include?(:autosave)
   end
 
+  def test_should_not_add_the_same_callbacks_multiple_times_for_has_one
+    assert_no_difference_when_adding_callbacks_twice_for Pirate, :ship
+  end
+
+  def test_should_not_add_the_same_callbacks_multiple_times_for_belongs_to
+    assert_no_difference_when_adding_callbacks_twice_for Ship, :pirate
+  end
+
+  def test_should_not_add_the_same_callbacks_multiple_times_for_has_many
+    assert_no_difference_when_adding_callbacks_twice_for Pirate, :birds
+  end
+
+  def test_should_not_add_the_same_callbacks_multiple_times_for_has_and_belongs_to_many
+    assert_no_difference_when_adding_callbacks_twice_for Pirate, :parrots
+  end
+
   private
 
   def base
     ActiveRecord::Base
   end
+
+  def assert_no_difference_when_adding_callbacks_twice_for(model, association_name)
+    reflection = model.reflect_on_association(association_name)
+    assert_no_difference "callbacks_for_model(#{model.name}).length" do
+      model.send(:add_autosave_association_callbacks, reflection)
+    end
+  end
+
+  def callbacks_for_model(model)
+    model.instance_variables.grep(/_callbacks$/).map do |ivar|
+      model.instance_variable_get(ivar)
+    end.flatten
+  end
 end
 
 class TestDefaultAutosaveAssociationOnAHasOneAssociation < ActiveRecord::TestCase
diff --git a/activerecord/test/cases/nested_attributes_test.rb b/activerecord/test/cases/nested_attributes_test.rb
index 53fd168e1b..5367907fb7 100644
--- a/activerecord/test/cases/nested_attributes_test.rb
+++ b/activerecord/test/cases/nested_attributes_test.rb
@@ -371,6 +371,15 @@ module NestedAttributesOnACollectionAssociationTests
     assert_respond_to @pirate, association_setter
   end
 
+  def test_should_save_only_one_association_on_create
+    pirate = Pirate.create!({
+      :catchphrase => 'Arr',
+      association_getter => { 'foo' => { :name => 'Grace OMalley' } }
+    })
+
+    assert_equal 1, pirate.reload.send(@association_name).count
+  end
+
   def test_should_take_a_hash_with_string_keys_and_assign_the_attributes_to_the_associated_models
     @alternate_params[association_getter].stringify_keys!
     @pirate.update_attributes @alternate_params