From e0d59e6219c752d8cffc6b78c2240755f5728922 Mon Sep 17 00:00:00 2001 From: Paul Nikitochkin Date: Mon, 8 Jul 2013 12:31:07 +0300 Subject: #4566: Remove extra decrement of transaction level `rollback_active_record_state!` tries to restore model state on `Exception` by invoking `restore_transaction_record_state` it decrement deep level by `1`. After restoring it ensure that states to be cleared and level decremented by invoking `clear_transaction_record_state`, which cause the bug: because state already reduced in `restore_transaction_record_state`. Removed double derement of transaction level and removed duplicated code which clear transaction state for top level. --- activerecord/CHANGELOG.md | 6 ++++++ activerecord/lib/active_record/transactions.rb | 6 +++--- activerecord/test/cases/transactions_test.rb | 14 ++++++++++++++ 3 files changed, 23 insertions(+), 3 deletions(-) diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 404443f4ad..624c26e759 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,9 @@ +* Remove extra decrement of transaction deep level. + + Fixes: #4566 + + *Paul Nikitochkin* + * Reset @column_defaults when assigning `locking_column`. We had a potential problem. For example: diff --git a/activerecord/lib/active_record/transactions.rb b/activerecord/lib/active_record/transactions.rb index 62920d936f..dcbf38a89f 100644 --- a/activerecord/lib/active_record/transactions.rb +++ b/activerecord/lib/active_record/transactions.rb @@ -304,6 +304,7 @@ module ActiveRecord run_callbacks :rollback ensure restore_transaction_record_state(force_restore_state) + clear_transaction_record_state end # Add the record to the current transaction so that the +after_rollback+ and +after_commit+ callbacks @@ -360,8 +361,8 @@ module ActiveRecord # Restore the new record state and id of a record that was previously saved by a call to save_record_state. def restore_transaction_record_state(force = false) #:nodoc: unless @_start_transaction_state.empty? - @_start_transaction_state[:level] = (@_start_transaction_state[:level] || 0) - 1 - if @_start_transaction_state[:level] < 1 || force + transaction_level = (@_start_transaction_state[:level] || 0) - 1 + if transaction_level < 1 || force restore_state = @_start_transaction_state was_frozen = restore_state[:frozen?] @attributes = @attributes.dup if @attributes.frozen? @@ -374,7 +375,6 @@ module ActiveRecord @attributes_cache.delete(self.class.primary_key) end @attributes.freeze if was_frozen - @_start_transaction_state.clear end end end diff --git a/activerecord/test/cases/transactions_test.rb b/activerecord/test/cases/transactions_test.rb index c0cad52b22..9c5f2e4724 100644 --- a/activerecord/test/cases/transactions_test.rb +++ b/activerecord/test/cases/transactions_test.rb @@ -117,6 +117,20 @@ class TransactionTest < ActiveRecord::TestCase assert !Topic.find(1).approved? end + def test_raising_exception_in_nested_transaction_restore_state_in_save + topic = Topic.new + + def topic.after_save_for_transaction + raise 'Make the transaction rollback' + end + + assert_raises(RuntimeError) do + Topic.transaction { topic.save } + end + + assert topic.new_record?, "#{topic.inspect} should be new record" + end + def test_update_should_rollback_on_failure author = Author.find(1) posts_count = author.posts.size -- cgit v1.2.3