let insert_record actuall save the object.

`before_add` callbacks are fired before the record is saved on
`has_and_belongs_to_many` assocations *and* on `has_many :through`
associations.  Before this change, `before_add` callbacks would be fired
before the record was saved on `has_and_belongs_to_many` associations, but
*not* on `has_many :through` associations.

Fixes #14144

activerecord/CHANGELOG.md

@@ -1,3 +1,11 @@
+* `before_add` callbacks are fired before the record is saved on
+  `has_and_belongs_to_many` assocations *and* on `has_many :through`
+  associations.  Before this change, `before_add` callbacks would be fired
+  before the record was saved on `has_and_belongs_to_many` associations, but
+  *not* on `has_many :through` associations.
+
+  Fixes #14144
+
 *   Fixed STI classes not defining an attribute method if there is a
     conflicting private method defined on its ancestors.
 

activerecord/lib/active_record/associations/collection_association.rb

@@ -513,13 +513,13 @@ module ActiveRecord
           target
         end
 
-        def concat_records(records)
+        def concat_records(records, should_raise = false)
           result = true
 
           records.flatten.each do |record|
             raise_on_type_mismatch!(record)
             add_to_target(record) do |rec|
-              result &&= insert_record(rec) unless owner.new_record?
+              result &&= insert_record(rec, true, should_raise) unless owner.new_record?
             end
           end
 

activerecord/lib/active_record/associations/has_many_through_association.rb

@@ -30,7 +30,6 @@ module ActiveRecord
         unless owner.new_record?
           records.flatten.each do |record|
             raise_on_type_mismatch!(record)
-            record.save! if record.new_record?
           end
         end
 
@@ -40,7 +39,7 @@ module ActiveRecord
       def concat_records(records)
         ensure_not_nested
 
-        records = super
+        records = super(records, true)
 
         if owner.new_record? && records
           records.flatten.each do |record|

activerecord/test/cases/associations/callbacks_test.rb

@@ -101,6 +101,27 @@ class AssociationCallbacksTest < ActiveRecord::TestCase
                   "after_adding#{david.id}"], ar.developers_log
   end
 
+  def test_has_and_belongs_to_many_before_add_called_before_save
+    dev     = nil
+    new_dev = nil
+    klass = Class.new(Project) do
+      def self.name; Project.name; end
+      has_and_belongs_to_many :developers_with_callbacks,
+                                :class_name => "Developer",
+                                :before_add => lambda { |o,r|
+        dev     = r
+        new_dev = r.new_record?
+      }
+    end
+    rec = klass.create!
+    alice = Developer.new(:name => 'alice')
+    rec.developers_with_callbacks << alice
+    assert_equal alice, dev
+    assert_not_nil new_dev
+    assert new_dev, "record should not have been saved"
+    assert_not alice.new_record?
+  end
+
   def test_has_and_belongs_to_many_after_add_called_after_save
     ar = projects(:active_record)
     assert ar.developers_log.empty?