Introduce transaction_joinable flag to mark that the fixtures transaction can't joined, a new savepoint is required even if :requires_new is not set. Use :requires_new option instead of :nest. Update changelog.

[#383 state:committed]

6 files changed, 45 insertions, 47 deletions

diff --git a/activerecord/CHANGELOG b/activerecord/CHANGELOG
index c750f486f9..35172ae110 100644
--- a/activerecord/CHANGELOG
+++ b/activerecord/CHANGELOG
@@ -1,5 +1,7 @@
 *2.3.0/3.0*
 
+* Support nested transactions using database savepoints.  #383 [Jonathan Viney, Hongli Lai]
+
 * Added dynamic scopes ala dynamic finders #1648 [Yaroslav Markin]
 
 * Fixed that ActiveRecord::Base#new_record? should return false (not nil) for existing records #1219 [Yaroslav Markin]
diff --git a/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb b/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb
index cecbc6b3ac..39118583bd 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb
@@ -89,14 +89,8 @@ module ActiveRecord
       # - The block will be run without doing anything. All database statements
       #   that happen within the block are effectively appended to the already
       #   open database transaction.
-      # - However, if +start_db_transaction+ is set to true, then the block will
-      #   be run inside a new database savepoint, effectively making the block
-      #   a sub-transaction.
-      # - If the #transactional_fixtures attribute is set to true, then the first
-      #   nested call to #transaction will create a new savepoint instead of
-      #   doing nothing. This makes it possible for toplevel transactions in unit
-      #   tests to behave like real transactions, even though a database
-      #   transaction has already been opened.
+      # - However, if +requires_new+ is set, the block will be wrapped in a
+      #   database savepoint acting as a sub-transaction.
       #
       # === Caveats
       #
@@ -111,20 +105,25 @@ module ActiveRecord
       # already-automatically-released savepoints:
       #
       #   Model.connection.transaction do  # BEGIN
-      #     Model.connection.transaction(true) do  # CREATE SAVEPOINT rails_savepoint_1
+      #     Model.connection.transaction(:requires_new => true) do  # CREATE SAVEPOINT active_record_1
       #       Model.connection.create_table(...)
-      #       # rails_savepoint_1 now automatically released
-      #     end  # RELEASE savepoint rails_savepoint_1  <--- BOOM! database error!
+      #       # active_record_1 now automatically released
+      #     end  # RELEASE SAVEPOINT active_record_1  <--- BOOM! database error!
       #   end
-      def transaction(start_db_transaction = false)
-        start_db_transaction ||= open_transactions == 0 || (open_transactions == 1 && transactional_fixtures)
+      def transaction(options = {})
+        options.assert_valid_keys :requires_new, :joinable
+
+        last_transaction_joinable, @transaction_joinable =
+          @transaction_joinable, options[:joinable] || true
+        requires_new = options[:requires_new] || !last_transaction_joinable
+
         transaction_open = false
         begin
           if block_given?
-            if start_db_transaction
+            if requires_new || open_transactions == 0
               if open_transactions == 0
                 begin_db_transaction
-              else
+              elsif requires_new
                 create_savepoint
               end
               increment_open_transactions
@@ -145,6 +144,8 @@ module ActiveRecord
           raise unless database_transaction_rollback.is_a? ActiveRecord::Rollback
         end
       ensure
+        @transaction_joinable = last_transaction_joinable
+
         if outside_transaction?
           @open_transactions = 0
         elsif transaction_open
diff --git a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb
index 5137b0f78c..a8cd9f033b 100755
--- a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb
@@ -165,24 +165,23 @@ module ActiveRecord
       def decrement_open_transactions
         @open_transactions -= 1
       end
-      
+
+      def transaction_joinable=(joinable)
+        @transaction_joinable = joinable
+      end
+
       def create_savepoint
       end
-      
+
       def rollback_to_savepoint
       end
-      
+
       def release_savepoint
       end
-      
+
       def current_savepoint_name
-        "rails_savepoint_#{open_transactions}"
+        "active_record_#{open_transactions}"
       end
-      
-      # Whether this AbstractAdapter is currently being used inside a unit test
-      # with transactional fixtures turned on. See DatabaseStatements#transaction
-      # for more information about the effect of this option.
-      attr_accessor :transactional_fixtures
 
       def log_info(sql, name, ms)
         if @logger && @logger.debug?
diff --git a/activerecord/lib/active_record/fixtures.rb b/activerecord/lib/active_record/fixtures.rb
index 039d5a4e8e..0131d9fac5 100644
--- a/activerecord/lib/active_record/fixtures.rb
+++ b/activerecord/lib/active_record/fixtures.rb
@@ -516,7 +516,7 @@ class Fixtures < (RUBY_VERSION < '1.9' ? YAML::Omap : Hash)
 
           all_loaded_fixtures.update(fixtures_map)
 
-          connection.transaction(connection.open_transactions.zero?) do
+          connection.transaction(:requires_new => true) do
             fixtures.reverse.each { |fixture| fixture.delete_existing_fixtures }
             fixtures.each { |fixture| fixture.insert_fixtures }
 
@@ -937,8 +937,8 @@ module ActiveRecord
           @@already_loaded_fixtures[self.class] = @loaded_fixtures
         end
         ActiveRecord::Base.connection.increment_open_transactions
+        ActiveRecord::Base.connection.transaction_joinable = false
         ActiveRecord::Base.connection.begin_db_transaction
-        ActiveRecord::Base.connection.transactional_fixtures = true
       # Load fixtures for every test.
       else
         Fixtures.reset_cache
@@ -961,7 +961,6 @@ module ActiveRecord
       if run_in_transaction? && ActiveRecord::Base.connection.open_transactions != 0
         ActiveRecord::Base.connection.rollback_db_transaction
         ActiveRecord::Base.connection.decrement_open_transactions
-        ActiveRecord::Base.connection.transactional_fixtures = false
       end
       ActiveRecord::Base.clear_active_connections!
     end
diff --git a/activerecord/lib/active_record/transactions.rb b/activerecord/lib/active_record/transactions.rb
index aaa298dc49..0b6e52c79b 100644
--- a/activerecord/lib/active_record/transactions.rb
+++ b/activerecord/lib/active_record/transactions.rb
@@ -137,14 +137,14 @@ module ActiveRecord
     #   
     #   User.find(:all)  # => empty
     #
-    # It is also possible to treat a certain #transaction call as its own
-    # sub-transaction, by passing <tt>:nest => true</tt> to #transaction. If
-    # anything goes wrong inside that transaction block, then the parent
-    # transaction will remain unaffected. For example:
+    # It is also possible to requires a sub-transaction by passing
+    # <tt>:requires_new => true</tt>.  If anything goes wrong, the
+    # database rolls back to the beginning of the sub-transaction
+    # without rolling back the parent transaction. For example:
     #
     #   User.transaction do
     #     User.create(:username => 'Kotori')
-    #     User.transaction(:nest => true) do
+    #     User.transaction(:requires_new => true) do
     #       User.create(:username => 'Nemu')
     #       raise ActiveRecord::Rollback
     #     end
@@ -169,20 +169,17 @@ module ActiveRecord
     # database error will occur because the savepoint has already been
     # automatically released. The following example demonstrates the problem:
     # 
-    #   Model.connection.transaction do          # BEGIN
-    #     Model.connection.transaction(true) do  # CREATE SAVEPOINT rails_savepoint_1
-    #       Model.connection.create_table(...)   # rails_savepoint_1 now automatically released
-    #     end                                    # RELEASE savepoint rails_savepoint_1
-    #                                            # ^^^^ BOOM! database error!
+    #   Model.connection.transaction do                           # BEGIN
+    #     Model.connection.transaction(:requires_new => true) do  # CREATE SAVEPOINT active_record_1
+    #       Model.connection.create_table(...)                    # active_record_1 now automatically released
+    #     end                                                     # RELEASE savepoint active_record_1
+    #                                                             # ^^^^ BOOM! database error!
     #   end
     module ClassMethods
       # See ActiveRecord::Transactions::ClassMethods for detailed documentation.
       def transaction(options = {}, &block)
-        options.assert_valid_keys :nest
-        
-        # See the API documentation for ConnectionAdapters::DatabaseStatements#transaction
-        # for useful information.
-        connection.transaction(options[:nest], &block)
+        # See the ConnectionAdapters::DatabaseStatements#transaction API docs.
+        connection.transaction(options, &block)
       end
     end
 
diff --git a/activerecord/test/cases/transactions_test.rb b/activerecord/test/cases/transactions_test.rb
index 0c69fee8f2..f07fad1828 100644
--- a/activerecord/test/cases/transactions_test.rb
+++ b/activerecord/test/cases/transactions_test.rb
@@ -228,7 +228,7 @@ class TransactionTest < ActiveRecord::TestCase
       @second.save!
 
       begin
-        Topic.transaction :nest => true do
+        Topic.transaction :requires_new => true do
           @first.happy = false
           @first.save!
           raise
@@ -268,17 +268,17 @@ class TransactionTest < ActiveRecord::TestCase
       @first.save!
       
       begin
-        Topic.transaction :nest => true do
+        Topic.transaction :requires_new => true do
           @first.content = "Two"
           @first.save!
           
           begin
-            Topic.transaction :nest => true do
+            Topic.transaction :requires_new => true do
               @first.content = "Three"
               @first.save!
               
               begin
-                Topic.transaction :nest => true do
+                Topic.transaction :requires_new => true do
                   @first.content = "Four"
                   @first.save!
                   raise