aboutsummaryrefslogtreecommitdiffstats
path: root/activerecord
diff options
context:
space:
mode:
authoreileencodes <eileencodes@gmail.com>2017-07-01 14:25:33 -0400
committereileencodes <eileencodes@gmail.com>2017-07-01 14:53:30 -0400
commit0237da287eb4c507d10a0c6d94150093acc52b03 (patch)
tree4248673d4f498716ed057a74320d08311d579e8a /activerecord
parent608ebccf8f6314c945444b400a37c2d07f21b253 (diff)
downloadrails-0237da287eb4c507d10a0c6d94150093acc52b03.tar.gz
rails-0237da287eb4c507d10a0c6d94150093acc52b03.tar.bz2
rails-0237da287eb4c507d10a0c6d94150093acc52b03.zip
Apply record state based on parent transaction state
Let's say you have a nested transaction and both records are saved. Before the outer transaction closes, a rollback is performed. Previously the record in the outer transaction would get marked as not persisted but the inner transaction would get persisted. ```ruby Post.transaction do post_one.save # will get rolled back Post.transaction(requires_new: true) do post_two.save # incorrectly remains marked as persisted end raise ActiveRecord::Rollback end ``` To fix this the PR changes transaction handling to have the child transaction ask the parent how the records should be marked. When there are child transactions, it will always be a SavpointTransaction because the stack isn't empty. From there we pass the parent_transaction to the child SavepointTransaction where we add the children to the parent so the parent can mark the inner transaction as rolledback and thus mark the record as not persisted. `update_attributes_from_transaction_state` uses the `completed?` check to correctly mark all the transactions as rolledback and the inner record as not persisted. ```ruby Post.transaction do post_one.save # will get rolled back Post.transaction(requires_new: true) do post_two.save # with new behavior, correctly marked as not persisted on rollback end raise ActiveRecord::Rollback end ``` Fixes #29320
Diffstat (limited to 'activerecord')
-rw-r--r--activerecord/CHANGELOG.md10
-rw-r--r--activerecord/lib/active_record/connection_adapters/abstract/transaction.rb17
-rw-r--r--activerecord/lib/active_record/transactions.rb2
-rw-r--r--activerecord/test/cases/transactions_test.rb70
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