From f4bae935abaccfcdbd4e184d07d58fd9f2b565bb Mon Sep 17 00:00:00 2001 From: Alec Larsen Date: Thu, 12 May 2016 11:42:00 -0500 Subject: [PATCH 1/2] Ensures destroy and restore operations are wrapped in a transaction that is rolled back when the callback chain is halted --- lib/destroyed_at.rb | 37 +++++++++++++++---------- lib/destroyed_at/record_not_restored.rb | 7 +++++ test/destroyed_at_test.rb | 36 ++++++++++++++++++++++++ test/test_helper.rb | 15 +++++++++- 4 files changed, 80 insertions(+), 15 deletions(-) create mode 100644 lib/destroyed_at/record_not_restored.rb diff --git a/lib/destroyed_at.rb b/lib/destroyed_at.rb index b1a4160..f3be2d5 100644 --- a/lib/destroyed_at.rb +++ b/lib/destroyed_at.rb @@ -2,6 +2,7 @@ require 'destroyed_at/belongs_to_association' require 'destroyed_at/has_many_association' require 'destroyed_at/has_one_association' +require 'destroyed_at/record_not_restored' require 'destroyed_at/mapper' module DestroyedAt @@ -39,12 +40,14 @@ def destroyed(time = nil) # Set an object's destroyed_at time. def destroy(timestamp = nil) - timestamp ||= @marked_for_destruction_at || current_time_from_proper_timezone - raw_write_attribute(:destroyed_at, timestamp) - run_callbacks(:destroy) do - destroy_associations - self.class.unscoped.where(self.class.primary_key => id).update_all(destroyed_at: timestamp) - @destroyed = true + with_transaction_returning_status do + timestamp ||= @marked_for_destruction_at || current_time_from_proper_timezone + raw_write_attribute(:destroyed_at, timestamp) + run_callbacks(:destroy) do + destroy_associations + self.class.unscoped.where(self.class.primary_key => id).update_all(destroyed_at: timestamp) + @destroyed = true + end end end @@ -56,16 +59,22 @@ def mark_for_destruction(timestamp = nil) # Set an object's destroyed_at time to nil. def restore - state = nil - run_callbacks(:restore) do - if state = (self.class.unscoped.where(self.class.primary_key => id).update_all(destroyed_at: nil) == 1) - _restore_associations - raw_write_attribute(:destroyed_at, nil) - @destroyed = false - true + with_transaction_returning_status do + state = nil + run_callbacks(:restore) do + if state = (self.class.unscoped.where(self.class.primary_key => id).update_all(destroyed_at: nil) == 1) + _restore_associations + raw_write_attribute(:destroyed_at, nil) + @destroyed = false + true + end end + state end - state + end + + def restore! + restore || raise(RecordNotRestored, self) end def persisted? diff --git a/lib/destroyed_at/record_not_restored.rb b/lib/destroyed_at/record_not_restored.rb new file mode 100644 index 0000000..f619730 --- /dev/null +++ b/lib/destroyed_at/record_not_restored.rb @@ -0,0 +1,7 @@ +class DestroyedAt::RecordNotRestored < ActiveRecord::ActiveRecordError + attr_reader :record + def initialize(record) + @record = record + super() + end +end \ No newline at end of file diff --git a/test/destroyed_at_test.rb b/test/destroyed_at_test.rb index cd798ca..34e603b 100644 --- a/test/destroyed_at_test.rb +++ b/test/destroyed_at_test.rb @@ -96,6 +96,18 @@ comment.destroyed_at.must_equal datetime end + it 'rollsback the transaction when destroying fails' do + pet_type = PetType.create(name: 'Cat') + pet = Pet.create(pet_type: pet_type) + + Pet.count.must_equal 1 + PetType.count.must_equal 1 + + pet_type.destroy + + Pet.count.must_equal 1 + PetType.count.must_equal 1 + end end describe 'restoring an activerecord instance' do @@ -192,6 +204,30 @@ post.comments.wont_include comment_1 post.comments.must_include comment_2 end + + it 'rollsback the transaction when restoring fails' do + pet_type = PetType.create(name: 'Cat') + + PetType.count.must_equal 1 + pet_type.destroy + + Pet.count.must_equal 0 + PetType.count.must_equal 0 + pet_type.restore + + Pet.count.must_equal 0 + PetType.count.must_equal 0 + end + + it 'raises DestroyedAt::RecordNotRestored if the record cannot be restored using #restore!' do + pet_type = PetType.create(name: 'Cat') + + PetType.count.must_equal 1 + pet_type.destroy + + PetType.count.must_equal 0 + assert_raises(DestroyedAt::RecordNotRestored) { pet_type.restore! } + end end describe 'deleting a record' do diff --git a/test/test_helper.rb b/test/test_helper.rb index d804fca..8780829 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -37,7 +37,8 @@ class << self ActiveRecord::Base.connection.execute(%{CREATE TABLE images (id INTEGER PRIMARY KEY, post_id INTEGER);}) ActiveRecord::Base.connection.execute(%{CREATE TABLE posts (id INTEGER PRIMARY KEY, author_id INTEGER, destroyed_at DATETIME);}) ActiveRecord::Base.connection.execute(%{CREATE TABLE people (id INTEGER PRIMARY KEY);}) -ActiveRecord::Base.connection.execute(%{CREATE TABLE pets (id INTEGER PRIMARY KEY, person_id INTEGER);}) +ActiveRecord::Base.connection.execute(%{CREATE TABLE pet_types (id INTEGER PRIMARY KEY, name TEXT, destroyed_at DATETIME);}) +ActiveRecord::Base.connection.execute(%{CREATE TABLE pets (id INTEGER PRIMARY KEY, person_id INTEGER, pet_type_id INTEGER);}) ActiveRecord::Base.connection.execute(%{CREATE TABLE likes (id INTEGER PRIMARY KEY, likeable_id INTEGER, likeable_type TEXT, destroyed_at DATETIME);}) class Author < ActiveRecord::Base @@ -52,6 +53,18 @@ class Person < ActiveRecord::Base class Pet < ActiveRecord::Base belongs_to :person + belongs_to :pet_type +end + +class PetType < ActiveRecord::Base + include DestroyedAt + has_many :pets, dependent: :restrict_with_error + + after_restore do + Pet.create!(pet_type: self) # mutate the database + + raise ActiveRecord::Rollback # halt the callback chain + end end class Avatar < ActiveRecord::Base From a6c2abf80324f9d7de9177fa78fac547a23fb8c3 Mon Sep 17 00:00:00 2001 From: Alec Larsen Date: Thu, 12 May 2016 12:33:56 -0500 Subject: [PATCH 2/2] Proves adding transaction support solves #56 --- test/destroyed_at_test.rb | 35 +++++++++++++++++++++++++++++++++++ test/test_helper.rb | 22 ++++++++++++++++++++++ 2 files changed, 57 insertions(+) diff --git a/test/destroyed_at_test.rb b/test/destroyed_at_test.rb index 34e603b..1489343 100644 --- a/test/destroyed_at_test.rb +++ b/test/destroyed_at_test.rb @@ -108,6 +108,41 @@ Pet.count.must_equal 1 PetType.count.must_equal 1 end + + it 'runs the after_commit callbacks when successful' do + pet_type = PetType.create(name: 'Cat') + + Pet.count.must_equal 0 + PetType.count.must_equal 1 + + pet_type.reset_transaction_state_callback_observers + + pet_type.after_commit_called?.must_equal false + pet_type.after_rollback_called?.must_equal false + + pet_type.destroy + + pet_type.after_commit_called?.must_equal true + pet_type.after_rollback_called?.must_equal false + end + + it 'runs the after_rollback callbacks when failed' do + pet_type = PetType.create(name: 'Cat') + pet = Pet.create(pet_type: pet_type) + + Pet.count.must_equal 1 + PetType.count.must_equal 1 + + pet_type.reset_transaction_state_callback_observers + + pet_type.after_commit_called?.must_equal false + pet_type.after_rollback_called?.must_equal false + + pet_type.destroy + + pet_type.after_commit_called?.must_equal false + pet_type.after_rollback_called?.must_equal true + end end describe 'restoring an activerecord instance' do diff --git a/test/test_helper.rb b/test/test_helper.rb index 8780829..4303b74 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -65,6 +65,28 @@ class PetType < ActiveRecord::Base raise ActiveRecord::Rollback # halt the callback chain end + + # Observe the transaction state callbacks + def reset_transaction_state_callback_observers + @after_commit_called = false + @after_rollback_called = false + end + + after_commit do + @after_commit_called = true + end + + def after_commit_called? + !!@after_commit_called + end + + after_rollback do + @after_rollback_called = true + end + + def after_rollback_called? + !!@after_rollback_called + end end class Avatar < ActiveRecord::Base