Merge pull request #16364 from arthurnn/make_transaction_one_class

Clarify Transaction responsibilities by breaking unneeded inheritance hierarchy.

2 files changed, 56 insertions, 72 deletions

diff --git a/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb b/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb
index 46aaaae2ec..4a7f2aaca8 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb
@@ -30,49 +30,24 @@ module ActiveRecord
       end
     end
 
-    class Transaction #:nodoc:
-      attr_reader :connection, :state
-
-      def initialize(connection)
-        @connection = connection
-        @state = TransactionState.new
-      end
-
-      def savepoint_name
-        nil
-      end
-    end
-
-    class NullTransaction < Transaction #:nodoc:
+    class NullTransaction #:nodoc:
       def initialize; end
       def closed?; true; end
       def open?; false; end
       def joinable?; false; end
-      # This is a noop when there are no open transactions
       def add_record(record); end
     end
 
-    class OpenTransaction < Transaction #:nodoc:
-      attr_reader :records
-      attr_writer :joinable
-
-      def initialize(connection, options = {})
-        super connection
-
-        @records   = []
-        @joinable  = options.fetch(:joinable, true)
-      end
-
-      def joinable?
-        @joinable
-      end
+    class Transaction #:nodoc:
 
-      def rollback
-        perform_rollback
-      end
+      attr_reader :connection, :state, :records, :savepoint_name
+      attr_writer :joinable
 
-      def commit
-        perform_commit
+      def initialize(connection, options)
+        @connection = connection
+        @state = TransactionState.new
+        @records = []
+        @joinable = options.fetch(:joinable, true)
       end
 
       def add_record(record)
@@ -83,19 +58,25 @@ module ActiveRecord
         end
       end
 
-      def rollback_records
+      def rollback
         @state.set_state(:rolledback)
+      end
+
+      def rollback_records
         records.uniq.each do |record|
           begin
-            record.rolledback!(self.is_a?(RealTransaction))
+            record.rolledback! full_rollback?
           rescue => e
             record.logger.error(e) if record.respond_to?(:logger) && record.logger
           end
         end
       end
 
-      def commit_records
+      def commit
         @state.set_state(:committed)
+      end
+
+      def commit_records
         records.uniq.each do |record|
           begin
             record.committed!
@@ -105,57 +86,57 @@ module ActiveRecord
         end
       end
 
-      def closed?
-        false
-      end
-
-      def open?
-        true
-      end
+      def full_rollback?; true; end
+      def joinable?; @joinable; end
+      def closed?; false; end
+      def open?; !closed?; end
     end
 
-    class RealTransaction < OpenTransaction #:nodoc:
-      def initialize(connection, _, options = {})
-        super(connection,  options)
+    class SavepointTransaction < Transaction
 
+      def initialize(connection, savepoint_name, options)
+        super(connection, options)
         if options[:isolation]
-          connection.begin_isolated_db_transaction(options[:isolation])
-        else
-          connection.begin_db_transaction
+          raise ActiveRecord::TransactionIsolationError, "cannot set transaction isolation in a nested transaction"
         end
+        connection.create_savepoint(@savepoint_name = savepoint_name)
       end
 
-      def perform_rollback
-        connection.rollback_db_transaction
+      def rollback
+        super
+        connection.rollback_to_savepoint(savepoint_name)
         rollback_records
       end
 
-      def perform_commit
-        connection.commit_db_transaction
-        commit_records
+      def commit
+        super
+        connection.release_savepoint(savepoint_name)
       end
+
+      def full_rollback?; false; end
     end
 
-    class SavepointTransaction < OpenTransaction #:nodoc:
-      attr_reader :savepoint_name
+    class RealTransaction < Transaction
 
-      def initialize(connection, savepoint_name, options = {})
+      def initialize(connection, options)
+        super
         if options[:isolation]
-          raise ActiveRecord::TransactionIsolationError, "cannot set transaction isolation in a nested transaction"
+          connection.begin_isolated_db_transaction(options[:isolation])
+        else
+          connection.begin_db_transaction
         end
-
-        super(connection, options)
-        connection.create_savepoint(@savepoint_name = savepoint_name)
       end
 
-      def perform_rollback
-        connection.rollback_to_savepoint(savepoint_name)
+      def rollback
+        super
+        connection.rollback_db_transaction
         rollback_records
       end
 
-      def perform_commit
-        @state.set_state(:committed)
-        connection.release_savepoint(savepoint_name)
+      def commit
+        super
+        connection.commit_db_transaction
+        commit_records
       end
     end
 
@@ -166,9 +147,12 @@ module ActiveRecord
       end
 
       def begin_transaction(options = {})
-        transaction_class = @stack.empty? ? RealTransaction : SavepointTransaction
-        transaction = transaction_class.new(@connection, "active_record_#{@stack.size}", options)
-
+        transaction =
+          if @stack.empty?
+            RealTransaction.new(@connection, options)
+          else
+            SavepointTransaction.new(@connection, "active_record_#{@stack.size}", options)
+          end
         @stack.push(transaction)
         transaction
       end
diff --git a/activerecord/test/cases/transactions_test.rb b/activerecord/test/cases/transactions_test.rb
index e518033192..b4849222b8 100644
--- a/activerecord/test/cases/transactions_test.rb
+++ b/activerecord/test/cases/transactions_test.rb
@@ -552,7 +552,7 @@ class TransactionTest < ActiveRecord::TestCase
     assert !transaction.state.rolledback?
     assert !transaction.state.committed?
 
-    transaction.perform_rollback
+    transaction.rollback
 
     assert transaction.state.rolledback?
     assert !transaction.state.committed?
@@ -566,7 +566,7 @@ class TransactionTest < ActiveRecord::TestCase
     assert !transaction.state.rolledback?
     assert !transaction.state.committed?
 
-    transaction.perform_commit
+    transaction.commit
 
     assert !transaction.state.rolledback?
     assert transaction.state.committed?