Rollback if commit raises an exception. Closes #8642.

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

activerecord/CHANGELOG

@@ -1,5 +1,7 @@
 *SVN*
 
+* Rollback if commit raises an exception.  #8642 [kik, Jeremy Kemper]
+
 * Update tests' use of fixtures for the new collections api.  #8726 [kamal]
 
 * Save associated records only if the association is already loaded.  #8713 [blaine]

activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb

@@ -57,7 +57,7 @@ module ActiveRecord
               transaction_open = true
             end
             yield
-        end
+          end
         rescue Exception => database_transaction_rollback
           if transaction_open
             transaction_open = false
@@ -66,7 +66,14 @@ module ActiveRecord
           raise unless database_transaction_rollback.is_a? ActiveRecord::Rollback
         end
       ensure
-        commit_db_transaction if transaction_open
+        if transaction_open
+          begin
+            commit_db_transaction
+          rescue Exception => database_transaction_rollback
+            rollback_db_transaction
+            raise
+          end
+        end
       end
 
       # Begins the transaction (and turns off auto-committing).

activerecord/test/transactions_test.rb

@@ -81,12 +81,12 @@ class TransactionTest < Test::Unit::TestCase
 
     assert @first.approved?, "First should still be changed in the objects"
     assert !@second.approved?, "Second should still be changed in the objects"
-    
+
     assert !Topic.find(1).approved?, "First shouldn't have been approved"
     assert Topic.find(2).approved?, "Second should still be approved"
   end
-  
-  
+
+
   def test_callback_rollback_in_save
     add_exception_raising_after_save_callback_to_topic
 
@@ -101,7 +101,7 @@ class TransactionTest < Test::Unit::TestCase
       remove_exception_raising_after_save_callback_to_topic
     end
   end
-  
+
   def test_callback_rollback_in_create
     new_topic = Topic.new(
       :title => "A new topic",
@@ -149,36 +149,48 @@ class TransactionTest < Test::Unit::TestCase
   end
 
   def test_manually_rolling_back_a_transaction
-    Topic.transaction do 
+    Topic.transaction do
       @first.approved  = true
       @second.approved = false
       @first.save
       @second.save
-      
+
       raise ActiveRecord::Rollback
     end
 
     assert @first.approved?, "First should still be changed in the objects"
     assert !@second.approved?, "Second should still be changed in the objects"
-    
+
     assert !Topic.find(1).approved?, "First shouldn't have been approved"
     assert Topic.find(2).approved?, "Second should still be approved"
   end
 
+  uses_mocha 'mocking connection.commit_db_transaction' do
+    def test_rollback_when_commit_raises
+      Topic.connection.expects(:commit_db_transaction).raises('OH NOES')
+      Topic.connection.expects(:rollback_db_transaction)
+
+      assert_raise RuntimeError do
+        Topic.transaction do
+          # do nothing
+        end
+      end
+    end
+  end
 
   private
     def add_exception_raising_after_save_callback_to_topic
       Topic.class_eval { def after_save() raise "Make the transaction rollback" end }
     end
-    
+
     def remove_exception_raising_after_save_callback_to_topic
       Topic.class_eval { remove_method :after_save }
     end
-    
+
     def add_exception_raising_after_create_callback_to_topic
       Topic.class_eval { def after_create() raise "Make the transaction rollback" end }
     end
-    
+
     def remove_exception_raising_after_create_callback_to_topic
       Topic.class_eval { remove_method :after_create }
     end