has_one should always remove the old record (properly), even if not saving the new record, so we don't get the database into a pickle

4 files changed, 31 insertions, 11 deletions

diff --git a/activerecord/lib/active_record/associations/has_one_association.rb b/activerecord/lib/active_record/associations/has_one_association.rb
index 739bb919c5..9135c60009 100644
--- a/activerecord/lib/active_record/associations/has_one_association.rb
+++ b/activerecord/lib/active_record/associations/has_one_association.rb
@@ -20,7 +20,7 @@ module ActiveRecord
         load_target
 
         if @target && @target != record
-          remove_target(save && @reflection.options[:dependent])
+          remove_target(@reflection.options[:dependent])
         end
 
         if record
diff --git a/activerecord/test/cases/associations/has_one_associations_test.rb b/activerecord/test/cases/associations/has_one_associations_test.rb
index 8203534a37..f004d46c98 100644
--- a/activerecord/test/cases/associations/has_one_associations_test.rb
+++ b/activerecord/test/cases/associations/has_one_associations_test.rb
@@ -2,9 +2,11 @@ require "cases/helper"
 require 'models/developer'
 require 'models/project'
 require 'models/company'
+require 'models/ship'
+require 'models/pirate'
 
 class HasOneAssociationsTest < ActiveRecord::TestCase
-  fixtures :accounts, :companies, :developers, :projects, :developers_projects
+  fixtures :accounts, :companies, :developers, :projects, :developers_projects, :ships, :pirates
 
   def setup
     Account.destroyed_account_ids.clear
@@ -164,15 +166,6 @@ class HasOneAssociationsTest < ActiveRecord::TestCase
     assert_equal account, firm.account
   end
 
-  def test_build_association_twice_without_saving_affects_nothing
-    count_of_account = Account.count
-    firm = Firm.find(:first)
-    firm.build_account("credit_limit" => 1000)
-    firm.build_account("credit_limit" => 2000)
-
-    assert_equal count_of_account, Account.count
-  end
-
   def test_create_association
     firm = Firm.create(:name => "GlobalMegaCorp")
     account = firm.create_account(:credit_limit => 1000)
@@ -293,4 +286,26 @@ class HasOneAssociationsTest < ActiveRecord::TestCase
     new_account = companies(:first_firm).build_account(:firm_name => 'Account')
     assert_equal new_account.firm_name, "Account"
   end
+
+  def test_creation_failure_without_dependent_option
+    pirate = pirates(:blackbeard)
+    orig_ship = pirate.ship.target
+
+    assert_equal ships(:black_pearl), orig_ship
+    new_ship = pirate.create_ship
+    assert_not_equal ships(:black_pearl), new_ship
+    assert_equal new_ship, pirate.ship
+    assert new_ship.new_record?
+    assert_nil orig_ship.pirate_id
+    assert !orig_ship.changed? # check it was saved
+  end
+
+  def test_creation_failure_with_dependent_option
+    pirate = pirates(:blackbeard).becomes(DestructivePirate)
+    orig_ship = pirate.dependent_ship.target
+
+    new_ship = pirate.create_dependent_ship
+    assert new_ship.new_record?
+    assert orig_ship.destroyed?
+  end
 end
diff --git a/activerecord/test/fixtures/ships.yml b/activerecord/test/fixtures/ships.yml
index 137055aad1..df914262b3 100644
--- a/activerecord/test/fixtures/ships.yml
+++ b/activerecord/test/fixtures/ships.yml
@@ -1,5 +1,6 @@
 black_pearl:
   name: "Black Pearl"
+  pirate: blackbeard
 interceptor:
   id: 2
   name: "Interceptor"
diff --git a/activerecord/test/models/pirate.rb b/activerecord/test/models/pirate.rb
index f2c45053e7..b0490f754e 100644
--- a/activerecord/test/models/pirate.rb
+++ b/activerecord/test/models/pirate.rb
@@ -78,3 +78,7 @@ class Pirate < ActiveRecord::Base
       ship_log << "#{callback}_#{record.class.name.downcase}_#{record.id || '<new>'}"
     end
 end
+
+class DestructivePirate < Pirate
+  has_one :dependent_ship, :class_name => 'Ship', :foreign_key => :pirate_id, :dependent => :destroy
+end