When assigning a has_one, if anything fails, the assignment should be rolled back entirely

2 files changed, 28 insertions, 21 deletions

diff --git a/activerecord/lib/active_record/associations/has_one_association.rb b/activerecord/lib/active_record/associations/has_one_association.rb
index 693c80163a..f60547c22c 100644
--- a/activerecord/lib/active_record/associations/has_one_association.rb
+++ b/activerecord/lib/active_record/associations/has_one_association.rb
@@ -19,23 +19,25 @@ module ActiveRecord
         raise_on_type_mismatch(record) unless record.nil?
         load_target
 
-        if @target && @target != record
-          remove_target!(@reflection.options[:dependent])
-        end
+        @reflection.klass.transaction do
+          if @target && @target != record
+            remove_target!(@reflection.options[:dependent])
+          end
+
+          if record
+            set_inverse_instance(record)
+            set_owner_attributes(record)
 
-        if record
-          set_owner_attributes(record)
-          set_inverse_instance(record)
+            if @owner.persisted? && save && !record.save
+              nullify_owner_attributes(record)
+              set_owner_attributes(@target)
+              raise RecordNotSaved, "Failed to save the new associated #{@reflection.name}."
+            end
+          end
         end
 
         @target = record
         loaded
-
-        if @owner.persisted? && record && save
-          unless record.save
-            raise RecordNotSaved, "Failed to save the new associated #{@reflection.name}."
-          end
-        end
       end
 
       private
@@ -59,17 +61,19 @@ module ActiveRecord
           if [:delete, :destroy].include?(method)
             @target.send(method)
           else
-            @target[@reflection.foreign_key] = nil
+            nullify_owner_attributes(@target)
 
-            if @target.persisted? && @owner.persisted?
-              unless @target.save
-                @target[@reflection.foreign_key] = @target.send("#{@reflection.foreign_key}_was")
-                raise RecordNotSaved, "Failed to remove the existing associated #{@reflection.name}. " +
-                                      "The record failed to save when after its foreign key was set to nil."
-              end
+            if @target.persisted? && @owner.persisted? && !@target.save
+              set_owner_attributes(@target)
+              raise RecordNotSaved, "Failed to remove the existing associated #{@reflection.name}. " +
+                                    "The record failed to save when after its foreign key was set to nil."
             end
           end
         end
+
+        def nullify_owner_attributes(record)
+          record[@reflection.foreign_key] = nil
+        end
     end
   end
 end
diff --git a/activerecord/test/cases/associations/has_one_associations_test.rb b/activerecord/test/cases/associations/has_one_associations_test.rb
index b9719fa983..925b76b901 100644
--- a/activerecord/test/cases/associations/has_one_associations_test.rb
+++ b/activerecord/test/cases/associations/has_one_associations_test.rb
@@ -6,6 +6,7 @@ require 'models/ship'
 require 'models/pirate'
 
 class HasOneAssociationsTest < ActiveRecord::TestCase
+  self.use_transactional_fixtures = false unless supports_savepoints?
   fixtures :accounts, :companies, :developers, :projects, :developers_projects, :ships, :pirates
 
   def setup
@@ -317,7 +318,9 @@ class HasOneAssociationsTest < ActiveRecord::TestCase
     assert_raise(ActiveRecord::RecordNotSaved) do
       pirate.ship = new_ship
     end
-    assert_equal new_ship, pirate.ship
-    assert_equal pirate.id, new_ship.pirate_id
+    assert_equal ships(:black_pearl), pirate.ship
+    assert_equal pirate.id, pirate.ship.pirate_id
+    assert_equal pirate.id, ships(:black_pearl).reload.pirate_id
+    assert_nil new_ship.pirate_id
   end
 end