Refactor association create and build so before & after callbacks behave consistently. Closes #8854.

git-svn-id: http://svn-commit.rubyonrails.org/rails/trunk@7935 5ecf4fe2-1ee6-0310-87b1-e25e094e27de

8 files changed, 94 insertions, 45 deletions

diff --git a/activerecord/CHANGELOG b/activerecord/CHANGELOG
index f9a9c58b60..8a3331a0ff 100644
--- a/activerecord/CHANGELOG
+++ b/activerecord/CHANGELOG
@@ -1,5 +1,7 @@
 *SVN*
 
+* Refactor association create and build so before & after callbacks behave consistently.  #8854 [lifofifo, mortent]
+
 * Quote table names. Defaults to column quoting.  #4593 [Justin Lynn, gwcoffey, eadz, Dmitry V. Sabanin, Jeremy Kemper]
 
 * Alias association #build to #new so it behaves predictably.  #8787 [lifofifo]
diff --git a/activerecord/lib/active_record/associations/association_collection.rb b/activerecord/lib/active_record/associations/association_collection.rb
index 5598d6daa7..5e2e2e0189 100644
--- a/activerecord/lib/active_record/associations/association_collection.rb
+++ b/activerecord/lib/active_record/associations/association_collection.rb
@@ -85,19 +85,15 @@ module ActiveRecord
       end
       
       def create(attrs = {})
-        ensure_owner_is_not_new
-        record = @reflection.klass.send(:with_scope, :create => construct_scope[:create]) { @reflection.klass.create(attrs) }                
-        @target ||= [] unless loaded?
-        @target << record 
-        record
+        if attrs.is_a?(Array)
+          attrs.collect { |attr| create(attr) }
+        else
+          create_record(attrs) { |record| record.save }
+        end
       end
 
       def create!(attrs = {})
-        ensure_owner_is_not_new
-        record = @reflection.klass.send(:with_scope, :create => construct_scope[:create]) { @reflection.klass.create!(attrs) }                
-        @target ||= [] unless loaded?
-        @target << record 
-        record
+        create_record(attrs) { |record| record.save! }
       end
 
       # Returns the size of the collection by executing a SELECT COUNT(*) query if the collection hasn't been loaded and
@@ -189,6 +185,27 @@ module ActiveRecord
         end
 
       private
+
+        def create_record(attrs, &block)
+          ensure_owner_is_not_new
+          record = @reflection.klass.send(:with_scope, :create => construct_scope[:create]) { @reflection.klass.new(attrs) }
+          add_record_to_target_with_callbacks(record, &block)
+        end
+
+        def build_record(attrs, &block)
+          record = @reflection.klass.new(attrs)
+          add_record_to_target_with_callbacks(record, &block)
+        end
+
+        def add_record_to_target_with_callbacks(record)
+          callback(:before_add, record)
+          yield(record) if block_given?
+          @target ||= [] unless loaded?
+          @target << record
+          callback(:after_add, record)
+          record
+        end
+
         def callback(method, record)
           callbacks_for(method).each do |callback|
             case callback
diff --git a/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb b/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb
index d67207fa4d..13c99455c0 100644
--- a/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb
+++ b/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb
@@ -8,33 +8,15 @@ module ActiveRecord
 
       def build(attributes = {})
         load_target
-        record = @reflection.klass.new(attributes)
-        @target << record
-        record
+        build_record(attributes)
       end
 
       def create(attributes = {})
-        # Can't use Base.create because the foreign key may be a protected attribute.
-        ensure_owner_is_not_new
-        if attributes.is_a?(Array)
-          attributes.collect { |attr| create(attr) }
-        else
-          record = build(attributes)
-          insert_record(record) unless @owner.new_record?
-          record
-        end
+        create_record(attributes) { |record| insert_record(record) }
       end
       
       def create!(attributes = {})
-        # Can't use Base.create! because the foreign key may be a protected attribute.
-        ensure_owner_is_not_new
-        if attributes.is_a?(Array)
-          attributes.collect { |attr| create(attr) }
-        else
-          record = build(attributes)
-          insert_record(record, true) unless @owner.new_record?
-          record
-        end        
+        create_record(attributes) { |record| insert_record(record, true) }
       end
 
       def find_first
@@ -160,6 +142,19 @@ module ActiveRecord
         def finding_with_ambiguous_select?(select_clause)
           !select_clause && @owner.connection.columns(@reflection.options[:join_table], "Join Table Columns").size != 2
         end
+
+      private
+        def create_record(attributes)
+          # Can't use Base.create because the foreign key may be a protected attribute.
+          ensure_owner_is_not_new
+          if attributes.is_a?(Array)
+            attributes.collect { |attr| create(attr) }
+          else
+            record = build(attributes)
+            yield(record)
+            record
+          end
+        end
     end
   end
 end
diff --git a/activerecord/lib/active_record/associations/has_many_association.rb b/activerecord/lib/active_record/associations/has_many_association.rb
index 282b43a2b7..b1b9895832 100644
--- a/activerecord/lib/active_record/associations/has_many_association.rb
+++ b/activerecord/lib/active_record/associations/has_many_association.rb
@@ -10,13 +10,7 @@ module ActiveRecord
         if attributes.is_a?(Array)
           attributes.collect { |attr| build(attr) }
         else
-          record = @reflection.klass.new(attributes)
-          set_belongs_to_association_for(record)
-          
-          @target ||= [] unless loaded?
-          @target << record
-          
-          record
+          build_record(attributes) { |record| set_belongs_to_association_for(record) }
         end
       end
 
diff --git a/activerecord/test/associations/callbacks_test.rb b/activerecord/test/associations/callbacks_test.rb
index afb1b8e84d..4b6b7f4123 100644
--- a/activerecord/test/associations/callbacks_test.rb
+++ b/activerecord/test/associations/callbacks_test.rb
@@ -60,6 +60,29 @@ class AssociationCallbacksTest < Test::Unit::TestCase
                   "after_adding#{@thinking.id}", "after_adding_proc#{@thinking.id}"], @david.post_log
   end
 
+  def test_has_many_callbacks_with_create
+    morten = Author.create :name => "Morten"
+    post = morten.posts_with_proc_callbacks.create! :title => "Hello", :body => "How are you doing?"
+    assert_equal ["before_adding<new>", "after_adding#{post.id}"], morten.post_log
+  end
+
+  def test_has_many_callbacks_with_create!
+    morten = Author.create! :name => "Morten"
+    post = morten.posts_with_proc_callbacks.create :title => "Hello", :body => "How are you doing?"
+    assert_equal ["before_adding<new>", "after_adding#{post.id}"], morten.post_log
+  end
+
+  def test_has_many_callbacks_for_save_on_parent
+    jack = Author.new :name => "Jack"
+    post = jack.posts_with_callbacks.build :title => "Call me back!", :body => "Before you wake up and after you sleep"
+
+    callback_log = ["before_adding<new>", "after_adding#{jack.posts_with_callbacks.first.id}"]
+    assert_equal callback_log, jack.post_log
+    assert jack.save
+    assert_equal 1, jack.posts_with_callbacks.count
+    assert_equal callback_log, jack.post_log
+  end
+
   def test_has_and_belongs_to_many_add_callback
     david = developers(:david)
     ar = projects(:active_record)
@@ -98,6 +121,17 @@ class AssociationCallbacksTest < Test::Unit::TestCase
     assert_equal log_array, activerecord.developers_log.sort
   end
 
+  def test_has_many_and_belongs_to_many_callbacks_for_save_on_parent
+    project = Project.new :name => "Callbacks"
+    project.developers_with_callbacks.build :name => "Jack", :salary => 95000
+
+    callback_log = ["before_adding<new>", "after_adding<new>"]
+    assert_equal callback_log, project.developers_log
+    assert project.save
+    assert_equal 1, project.developers_with_callbacks.count
+    assert_equal callback_log, project.developers_log
+  end
+
   def test_dont_add_if_before_callback_raises_exception
     assert !@david.unchangable_posts.include?(@authorless)
     begin
diff --git a/activerecord/test/associations_test.rb b/activerecord/test/associations_test.rb
index 9e4d0f2711..f912816f33 100755
--- a/activerecord/test/associations_test.rb
+++ b/activerecord/test/associations_test.rb
@@ -595,6 +595,13 @@ class HasManyAssociationsTest < Test::Unit::TestCase
     end
   end
   
+  def test_create_with_bang_on_has_many_raises_when_record_not_saved
+    assert_raises(ActiveRecord::RecordInvalid) do
+      firm = Firm.find(:first)
+      firm.plain_clients.create!
+    end
+  end
+
   def test_create_with_bang_on_habtm_when_parent_is_new_raises
     assert_raises(ActiveRecord::RecordNotSaved) do 
       Developer.new("name" => "Aredridel").projects.create!    
diff --git a/activerecord/test/fixtures/author.rb b/activerecord/test/fixtures/author.rb
index fa710bab9c..5ef0c6cfe9 100644
--- a/activerecord/test/fixtures/author.rb
+++ b/activerecord/test/fixtures/author.rb
@@ -33,13 +33,13 @@ class Author < ActiveRecord::Base
            :before_remove => :log_before_removing,
            :after_remove  => :log_after_removing
   has_many :posts_with_proc_callbacks, :class_name => "Post",
-           :before_add    => Proc.new {|o, r| o.post_log << "before_adding#{r.id}"},
-           :after_add     => Proc.new {|o, r| o.post_log << "after_adding#{r.id}"},
+           :before_add    => Proc.new {|o, r| o.post_log << "before_adding#{r.id || '<new>'}"},
+           :after_add     => Proc.new {|o, r| o.post_log << "after_adding#{r.id || '<new>'}"},
            :before_remove => Proc.new {|o, r| o.post_log << "before_removing#{r.id}"},
            :after_remove  => Proc.new {|o, r| o.post_log << "after_removing#{r.id}"}
   has_many :posts_with_multiple_callbacks, :class_name => "Post",
-           :before_add => [:log_before_adding, Proc.new {|o, r| o.post_log << "before_adding_proc#{r.id}"}],
-           :after_add  => [:log_after_adding,  Proc.new {|o, r| o.post_log << "after_adding_proc#{r.id}"}]
+           :before_add => [:log_before_adding, Proc.new {|o, r| o.post_log << "before_adding_proc#{r.id || '<new>'}"}],
+           :after_add  => [:log_after_adding,  Proc.new {|o, r| o.post_log << "after_adding_proc#{r.id || '<new>'}"}]
   has_many :unchangable_posts, :class_name => "Post", :before_add => :raise_exception, :after_add => :log_after_adding
 
   has_many :categorizations
@@ -74,7 +74,7 @@ class Author < ActiveRecord::Base
 
   private
     def log_before_adding(object)
-      @post_log << "before_adding#{object.id}"
+      @post_log << "before_adding#{object.id || '<new>'}"
     end
 
     def log_after_adding(object)
diff --git a/activerecord/test/fixtures/project.rb b/activerecord/test/fixtures/project.rb
index 2079dc9b58..2538b99d00 100644
--- a/activerecord/test/fixtures/project.rb
+++ b/activerecord/test/fixtures/project.rb
@@ -7,8 +7,8 @@ class Project < ActiveRecord::Base
   has_and_belongs_to_many :salaried_developers, :class_name => "Developer", :conditions => "salary > 0"
   has_and_belongs_to_many :developers_with_finder_sql, :class_name => "Developer", :finder_sql => 'SELECT t.*, j.* FROM developers_projects j, developers t WHERE t.id = j.developer_id AND j.project_id = #{id}'
   has_and_belongs_to_many :developers_by_sql, :class_name => "Developer", :delete_sql => "DELETE FROM developers_projects WHERE project_id = \#{id} AND developer_id = \#{record.id}"
-  has_and_belongs_to_many :developers_with_callbacks, :class_name => "Developer", :before_add => Proc.new {|o, r| o.developers_log << "before_adding#{r.id}"},
-                            :after_add => Proc.new {|o, r| o.developers_log << "after_adding#{r.id}"}, 
+  has_and_belongs_to_many :developers_with_callbacks, :class_name => "Developer", :before_add => Proc.new {|o, r| o.developers_log << "before_adding#{r.id || '<new>'}"},
+                            :after_add => Proc.new {|o, r| o.developers_log << "after_adding#{r.id || '<new>'}"},
                             :before_remove => Proc.new {|o, r| o.developers_log << "before_removing#{r.id}"},
                             :after_remove => Proc.new {|o, r| o.developers_log << "after_removing#{r.id}"}