0
@@ -1,5 +1,7 @@
0
 *Edge*
0
 
0
+* before_save, before_validation and before_destroy callbacks that return false will now ROLLBACK the transaction.  Previously this would have been committed before the processing was aborted. #891 [Xavier Noria]
0
+
0
 * Transactional migrations for databases which support them.  #834 [divoxx, Adam Wiggins, Tarmo Tänav]
0
 
0
 * Set config.active_record.timestamped_migrations = false to have migrations with numeric prefix instead of UTC timestamp. #446. [Andrew Stone, Nik Wakelin]

0
@@ -169,6 +169,18 @@ module ActiveRecord
0
   # If a <tt>before_*</tt> callback returns +false+, all the later callbacks and the associated action are cancelled. If an <tt>after_*</tt> callback returns
0
   # +false+, all the later callbacks are cancelled. Callbacks are generally run in the order they are defined, with the exception of callbacks
0
   # defined as methods on the model, which are called last.
0
+  #
0
+  # == Transactions
0
+  #
0
+  # The entire callback chain of a +save+, <tt>save!</tt>, or +destroy+ call runs
0
+  # within a transaction. That includes <tt>after_*</tt> hooks. If everything
0
+  # goes fine a COMMIT is executed once the chain has been completed.
0
+  #
0
+  # If a <tt>before_*</tt> callback cancels the action a ROLLBACK is issued. You
0
+  # can also trigger a ROLLBACK raising an exception in any of the callbacks,
0
+  # including <tt>after_*</tt> hooks. Note, however, that in that case the client
0
+  # needs to be aware of it because an ordinary +save+ will raise such exception
0
+  # instead of quietly returning +false+.
0
   module Callbacks
0
     CALLBACKS = %w(
0
       after_find after_initialize before_save after_save before_create after_create before_update after_update before_validation

0
@@ -91,11 +91,11 @@ module ActiveRecord
0
     end
0
 
0
     def destroy_with_transactions #:nodoc:
0
-      transaction { destroy_without_transactions }
0
+      with_transaction_returning_status(:destroy_without_transactions)
0
     end
0
 
0
     def save_with_transactions(perform_validation = true) #:nodoc:
0
-      rollback_active_record_state! { transaction { save_without_transactions(perform_validation) } }
0
+      rollback_active_record_state! { with_transaction_returning_status(:save_without_transactions, perform_validation) }
0
     end
0
 
0
     def save_with_transactions! #:nodoc:
0
@@ -118,5 +118,17 @@ module ActiveRecord
0
       end
0
       raise
0
     end
0
+
0
+    # Executes +method+ within a transaction and captures its return value as a
0
+    # status flag. If the status is true the transaction is committed, otherwise
0
+    # a ROLLBACK is issued. In any case the status flag is returned.
0
+    def with_transaction_returning_status(method, *args)
0
+      status = nil
0
+      transaction do
0
+        status = send(method, *args)
0
+        raise ActiveRecord::Rollback unless status
0
+      end
0
+      status
0
+    end
0
   end
0
 end

0
@@ -2,6 +2,7 @@ require "cases/helper"
0
 require 'models/topic'
0
 require 'models/reply'
0
 require 'models/developer'
0
+require 'models/book'
0
 
0
 class TransactionTest < ActiveRecord::TestCase
0
   self.use_transactional_fixtures = false
0
@@ -86,8 +87,7 @@ class TransactionTest < ActiveRecord::TestCase
0
     assert Topic.find(2).approved?, "Second should still be approved"
0
   end
0
 
0
-
0
-  def test_callback_rollback_in_save
0
+  def test_raising_exception_in_callback_rollbacks_in_save
0
     add_exception_raising_after_save_callback_to_topic
0
 
0
     begin
0
@@ -102,6 +102,54 @@ class TransactionTest < ActiveRecord::TestCase
0
     end
0
   end
0
 
0
+  def test_cancellation_from_before_destroy_rollbacks_in_destroy
0
+    add_cancelling_before_destroy_with_db_side_effect_to_topic
0
+    begin
0
+      nbooks_before_destroy = Book.count
0
+      status = @first.destroy
0
+      assert !status
0
+      assert_nothing_raised(ActiveRecord::RecordNotFound) { @first.reload }
0
+      assert_equal nbooks_before_destroy, Book.count
0
+    ensure
0
+      remove_cancelling_before_destroy_with_db_side_effect_to_topic
0
+    end
0
+  end
0
+
0
+  def test_cancellation_from_before_filters_rollbacks_in_save
0
+    %w(validation save).each do |filter|
0
+      send("add_cancelling_before_#{filter}_with_db_side_effect_to_topic")
0
+      begin
0
+        nbooks_before_save = Book.count
0
+        original_author_name = @first.author_name
0
+        @first.author_name += '_this_should_not_end_up_in_the_db'
0
+        status = @first.save
0
+        assert !status
0
+        assert_equal original_author_name, @first.reload.author_name
0
+        assert_equal nbooks_before_save, Book.count
0
+      ensure
0
+        send("remove_cancelling_before_#{filter}_with_db_side_effect_to_topic")
0
+      end
0
+    end
0
+  end
0
+
0
+  def test_cancellation_from_before_filters_rollbacks_in_save!
0
+    %w(validation save).each do |filter|
0
+      send("add_cancelling_before_#{filter}_with_db_side_effect_to_topic")
0
+      begin
0
+        nbooks_before_save = Book.count
0
+        original_author_name = @first.author_name
0
+        @first.author_name += '_this_should_not_end_up_in_the_db'
0
+        @first.save!
0
+        flunk
0
+      rescue => e
0
+        assert_equal original_author_name, @first.reload.author_name
0
+        assert_equal nbooks_before_save, Book.count
0
+      ensure
0
+        send("remove_cancelling_before_#{filter}_with_db_side_effect_to_topic")
0
+      end
0
+    end
0
+  end
0
+
0
   def test_callback_rollback_in_create
0
     new_topic = Topic.new(
0
       :title => "A new topic",
0
@@ -221,6 +269,16 @@ class TransactionTest < ActiveRecord::TestCase
0
     def remove_exception_raising_after_create_callback_to_topic
0
       Topic.class_eval { remove_method :after_create }
0
     end
0
+
0
+    %w(validation save destroy).each do |filter|
0
+      define_method("add_cancelling_before_#{filter}_with_db_side_effect_to_topic") do
0
+        Topic.class_eval "def before_#{filter}() Book.create; false end"
0
+      end
0
+
0
+      define_method("remove_cancelling_before_#{filter}_with_db_side_effect_to_topic") do
0
+        Topic.class_eval "remove_method :before_#{filter}"
0
+      end
0
+    end
0
 end
0
 
0
 if current_adapter?(:PostgreSQLAdapter)