has_many :through create should not raise validation errors

[#2934 state:committed]

Signed-off-by: Jeremy Kemper <jeremy@bitsweat.net>

6 files changed, 45 insertions, 4 deletions

diff --git a/activerecord/lib/active_record/associations/has_many_through_association.rb b/activerecord/lib/active_record/associations/has_many_through_association.rb
index f4507c979c..292da58831 100644
--- a/activerecord/lib/active_record/associations/has_many_through_association.rb
+++ b/activerecord/lib/active_record/associations/has_many_through_association.rb
@@ -20,7 +20,11 @@ module ActiveRecord
         ensure_owner_is_not_new
 
         transaction do
-          self << (object = attrs ? @reflection.klass.send(:with_scope, :create => attrs) { @reflection.create_association } : @reflection.create_association)
+          object = attrs ? @reflection.klass.send(:with_scope, :create => attrs) { @reflection.create_association } : @reflection.create_association
+          raise_on_type_mismatch(object)
+          add_record_to_target_with_callbacks(object) do |r|
+            insert_record(object, false)
+          end
           object
         end
       end
diff --git a/activerecord/test/cases/associations/has_many_through_associations_test.rb b/activerecord/test/cases/associations/has_many_through_associations_test.rb
index 58ce2ca3a5..59985374d3 100644
--- a/activerecord/test/cases/associations/has_many_through_associations_test.rb
+++ b/activerecord/test/cases/associations/has_many_through_associations_test.rb
@@ -11,9 +11,12 @@ require 'models/author'
 require 'models/owner'
 require 'models/pet'
 require 'models/toy'
+require 'models/contract'
+require 'models/company'
+require 'models/developer'
 
 class HasManyThroughAssociationsTest < ActiveRecord::TestCase
-  fixtures :posts, :readers, :people, :comments, :authors, :owners, :pets, :toys, :jobs, :references
+  fixtures :posts, :readers, :people, :comments, :authors, :owners, :pets, :toys, :jobs, :references, :companies
 
   def test_associate_existing
     assert_queries(2) { posts(:thinking);people(:david) }
@@ -176,6 +179,30 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase
     assert_raises(ActiveRecord::RecordNotSaved) { p.people.create!(:first_name => "snow") }
   end
 
+  def test_associate_with_create_and_invalid_options
+    peeps = companies(:first_firm).developers.count
+    assert_nothing_raised { companies(:first_firm).developers.create(:name => '0') }
+    assert_equal peeps, companies(:first_firm).developers.count
+  end
+
+  def test_associate_with_create_and_valid_options
+    peeps = companies(:first_firm).developers.count
+    assert_nothing_raised { companies(:first_firm).developers.create(:name => 'developer') }
+    assert_equal peeps + 1, companies(:first_firm).developers.count
+  end
+
+  def test_associate_with_create_bang_and_invalid_options
+    peeps = companies(:first_firm).developers.count
+    assert_raises(ActiveRecord::RecordInvalid) { companies(:first_firm).developers.create!(:name => '0') }
+    assert_equal peeps, companies(:first_firm).developers.count
+  end
+
+  def test_associate_with_create_bang_and_valid_options
+    peeps = companies(:first_firm).developers.count
+    assert_nothing_raised { companies(:first_firm).developers.create!(:name => 'developer') }
+    assert_equal peeps + 1, companies(:first_firm).developers.count
+  end
+
   def test_clear_associations
     assert_queries(2) { posts(:welcome);posts(:welcome).people(true) }
 
diff --git a/activerecord/test/cases/reflection_test.rb b/activerecord/test/cases/reflection_test.rb
index 4083b990d9..a164f5e060 100644
--- a/activerecord/test/cases/reflection_test.rb
+++ b/activerecord/test/cases/reflection_test.rb
@@ -176,8 +176,8 @@ class ReflectionTest < ActiveRecord::TestCase
 
   def test_reflection_of_all_associations
     # FIXME these assertions bust a lot
-    assert_equal 29, Firm.reflect_on_all_associations.size
-    assert_equal 22, Firm.reflect_on_all_associations(:has_many).size
+    assert_equal 31, Firm.reflect_on_all_associations.size
+    assert_equal 24, Firm.reflect_on_all_associations(:has_many).size
     assert_equal 7, Firm.reflect_on_all_associations(:has_one).size
     assert_equal 0, Firm.reflect_on_all_associations(:belongs_to).size
   end
diff --git a/activerecord/test/models/company.rb b/activerecord/test/models/company.rb
index 1c05e523e0..ab09f88a9f 100644
--- a/activerecord/test/models/company.rb
+++ b/activerecord/test/models/company.rb
@@ -9,6 +9,8 @@ class Company < AbstractCompany
   validates_presence_of :name
 
   has_one :dummy_account, :foreign_key => "firm_id", :class_name => "Account"
+  has_many :contracts
+  has_many :developers, :through => :contracts
 
   def arbitrary_method
     "I am Jack's profound disappointment"
diff --git a/activerecord/test/models/contract.rb b/activerecord/test/models/contract.rb
new file mode 100644
index 0000000000..606c99cd4e
--- /dev/null
+++ b/activerecord/test/models/contract.rb
@@ -0,0 +1,4 @@
+class Contract < ActiveRecord::Base
+  belongs_to :company
+  belongs_to :developer
+end
\ No newline at end of file
diff --git a/activerecord/test/schema/schema.rb b/activerecord/test/schema/schema.rb
index 5f60d5e137..9ab4cf6f43 100644
--- a/activerecord/test/schema/schema.rb
+++ b/activerecord/test/schema/schema.rb
@@ -131,6 +131,10 @@ ActiveRecord::Schema.define do
     t.integer :extendedWarranty, :null => false
   end
 
+  create_table :contracts, :force => true do |t|
+    t.integer :developer_id
+    t.integer :company_id
+  end
 
   create_table :customers, :force => true do |t|
     t.string  :name