diff options
| -rw-r--r-- | activerecord/CHANGELOG.md | 10 | ||||
| -rw-r--r-- | activerecord/lib/active_record/connection_adapters/abstract/transaction.rb | 17 | ||||
| -rw-r--r-- | activerecord/lib/active_record/transactions.rb | 2 | ||||
| -rw-r--r-- | activerecord/test/cases/transactions_test.rb | 70 |
4 files changed, 96 insertions, 3 deletions
diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index cc1dd324a3..3255eec486 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,13 @@ +* Fix transactions to apply state to child transactions + + Previously if you had a nested transaction and the outer transaction was rolledback the record from the + inner transaction would still be marked as persisted. + + This change fixes that by applying the state of the parent transaction to the child transaction when the + parent transaction is rolledback. This will correctly mark records from the inner transaction as not persisted. + + *Eileen M. Uchitelle*, *Aaron Patterson* + * Deprecate `set_state` method in `TransactionState` Deprecated the `set_state` method in favor of setting the state via specific methods. If you need to mark the diff --git a/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb b/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb index 265775c15c..2393e35ecd 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb @@ -3,6 +3,11 @@ module ActiveRecord class TransactionState def initialize(state = nil) @state = state + @children = [] + end + + def add_child(state) + @children << state end def finalized? @@ -17,6 +22,10 @@ module ActiveRecord @state == :rolledback end + def fully_completed? + completed? + end + def completed? committed? || rolledback? end @@ -40,6 +49,7 @@ module ActiveRecord end def rollback! + @children.each { |c| c.rollback! } @state = :rolledback end @@ -121,8 +131,11 @@ module ActiveRecord end class SavepointTransaction < Transaction - def initialize(connection, savepoint_name, options, *args) + def initialize(connection, savepoint_name, parent_transaction, options, *args) super(connection, options, *args) + + parent_transaction.state.add_child(@state) + if options[:isolation] raise ActiveRecord::TransactionIsolationError, "cannot set transaction isolation in a nested transaction" end @@ -176,7 +189,7 @@ module ActiveRecord if @stack.empty? RealTransaction.new(@connection, options, run_commit_callbacks: run_commit_callbacks) else - SavepointTransaction.new(@connection, "active_record_#{@stack.size}", options, + SavepointTransaction.new(@connection, "active_record_#{@stack.size}", @stack.last, options, run_commit_callbacks: run_commit_callbacks) end diff --git a/activerecord/lib/active_record/transactions.rb b/activerecord/lib/active_record/transactions.rb index 45795fa287..463bb1f314 100644 --- a/activerecord/lib/active_record/transactions.rb +++ b/activerecord/lib/active_record/transactions.rb @@ -490,7 +490,7 @@ module ActiveRecord def update_attributes_from_transaction_state(transaction_state) if transaction_state && transaction_state.finalized? restore_transaction_record_state if transaction_state.rolledback? - clear_transaction_record_state + clear_transaction_record_state if transaction_state.fully_completed? end end end diff --git a/activerecord/test/cases/transactions_test.rb b/activerecord/test/cases/transactions_test.rb index ce0399fd30..82c369f252 100644 --- a/activerecord/test/cases/transactions_test.rb +++ b/activerecord/test/cases/transactions_test.rb @@ -304,6 +304,76 @@ class TransactionTest < ActiveRecord::TestCase assert !Topic.find(2).approved?, "Second should have been unapproved" end + def test_nested_transaction_with_new_transaction_applies_parent_state_on_rollback + topic_one = Topic.new(title: "A new topic") + topic_two = Topic.new(title: "Another new topic") + + Topic.transaction do + topic_one.save + + Topic.transaction(requires_new: true) do + topic_two.save + + assert_predicate topic_one, :persisted? + assert_predicate topic_two, :persisted? + end + + raise ActiveRecord::Rollback + end + + refute_predicate topic_one, :persisted? + refute_predicate topic_two, :persisted? + end + + def test_nested_transaction_without_new_transaction_applies_parent_state_on_rollback + topic_one = Topic.new(title: "A new topic") + topic_two = Topic.new(title: "Another new topic") + + Topic.transaction do + topic_one.save + + Topic.transaction do + topic_two.save + + assert_predicate topic_one, :persisted? + assert_predicate topic_two, :persisted? + end + + raise ActiveRecord::Rollback + end + + refute_predicate topic_one, :persisted? + refute_predicate topic_two, :persisted? + end + + def test_double_nested_transaction_applies_parent_state_on_rollback + topic_one = Topic.new(title: "A new topic") + topic_two = Topic.new(title: "Another new topic") + topic_three = Topic.new(title: "Another new topic of course") + + Topic.transaction do + topic_one.save + + Topic.transaction do + topic_two.save + + Topic.transaction do + topic_three.save + end + end + + assert_predicate topic_one, :persisted? + assert_predicate topic_two, :persisted? + assert_predicate topic_three, :persisted? + + raise ActiveRecord::Rollback + end + + refute_predicate topic_one, :persisted? + refute_predicate topic_two, :persisted? + refute_predicate topic_three, :persisted? + end + def test_manually_rolling_back_a_transaction Topic.transaction do @first.approved = true |