Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
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
  • Loading branch information
eileencodes committed Jul 1, 2017
1 parent 608ebcc commit 0237da2
Show file tree
Hide file tree
Showing 4 changed files with 96 additions and 3 deletions.
10 changes: 10 additions & 0 deletions 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
Expand Down
Expand Up @@ -3,6 +3,11 @@ module ConnectionAdapters
class TransactionState
def initialize(state = nil)
@state = state
@children = []
end

def add_child(state)
@children << state
end

def finalized?
Expand All @@ -17,6 +22,10 @@ def rolledback?
@state == :rolledback
end

def fully_completed?
completed?
end

def completed?
committed? || rolledback?
end
Expand All @@ -40,6 +49,7 @@ def set_state(state)
end

def rollback!
@children.each { |c| c.rollback! }
@state = :rolledback
end

Expand Down Expand Up @@ -121,8 +131,11 @@ def open?; !closed?; end
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
Expand Down Expand Up @@ -176,7 +189,7 @@ def begin_transaction(options = {})
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

Expand Down
2 changes: 1 addition & 1 deletion activerecord/lib/active_record/transactions.rb
Expand Up @@ -490,7 +490,7 @@ def sync_with_transaction_state
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
Expand Down
70 changes: 70 additions & 0 deletions activerecord/test/cases/transactions_test.rb
Expand Up @@ -304,6 +304,76 @@ def test_nested_explicit_transactions
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
Expand Down

0 comments on commit 0237da2

Please sign in to comment.