From 15bebc4982c8c5a57c4606d8531ebbb2b7acbe46 Mon Sep 17 00:00:00 2001 From: Aymeric Le Dorze Date: Wed, 1 Aug 2018 13:57:56 +0200 Subject: [PATCH] Fix many callback problems such as callbacks not being called or being called too many times. --- lib/acts_as_paranoid/core.rb | 29 ++++++++++++ test/test_core.rb | 86 ++++++++++++++++++++++++++++++------ test/test_helper.rb | 15 +++---- 3 files changed, 108 insertions(+), 22 deletions(-) diff --git a/lib/acts_as_paranoid/core.rb b/lib/acts_as_paranoid/core.rb index 08464c77..0c48d7c4 100644 --- a/lib/acts_as_paranoid/core.rb +++ b/lib/acts_as_paranoid/core.rb @@ -4,6 +4,12 @@ module ActsAsParanoid module Core def self.included(base) base.extend ClassMethods + if ActiveRecord::VERSION::MAJOR == 5 + base.send(:alias_method, :committed_without_paranoid!, :committed!) + base.send(:alias_method, :committed!, :committed_with_paranoid!) + base.send(:alias_method, :remember_transaction_record_state_without_paranoid, :remember_transaction_record_state) + base.send(:alias_method, :remember_transaction_record_state, :remember_transaction_record_state_with_paranoid) + end end module ClassMethods @@ -142,6 +148,8 @@ def destroy_fully! decrement_counters_on_associations end + @_trigger_destroy_callback = true + stale_paranoid_value @destroyed = true freeze @@ -253,6 +261,14 @@ def deleted_fully? alias destroyed_fully? deleted_fully? + def committed_with_paranoid!(should_run_callbacks: true) + committed_without_paranoid!(should_run_callbacks: should_run_callbacks) + ensure + if defined?(@_trigger_destroy_callback) && !@_trigger_destroy_callback.nil? && !@destroyed && deleted? + @_trigger_destroy_callback = nil + end + end + private def get_association_scope(reflection:) @@ -302,5 +318,18 @@ def stale_paranoid_value self.paranoid_value = self.class.delete_now_value clear_attribute_changes([self.class.paranoid_column]) end + + if ActiveRecord::VERSION::MAJOR == 5 + def remember_transaction_record_state_with_paranoid + remember_transaction_record_state_without_paranoid + remember_trigger_destroy_callback_before_last_commit + end + + def remember_trigger_destroy_callback_before_last_commit + if _committed_already_called && defined?(@_trigger_destroy_callback) && !@_trigger_destroy_callback.nil? && !@destroyed && deleted? + @_trigger_destroy_callback = nil + end + end + end end end diff --git a/test/test_core.rb b/test/test_core.rb index 5dcc257f..6822e52a 100644 --- a/test/test_core.rb +++ b/test/test_core.rb @@ -366,21 +366,32 @@ def test_paranoid_destroy_callbacks @paranoid_with_callback.destroy end - assert @paranoid_with_callback.called_before_destroy - assert @paranoid_with_callback.called_after_destroy - assert @paranoid_with_callback.called_after_commit_on_destroy + assert_equal 1, @paranoid_with_callback.called_before_destroy + assert_equal 1, @paranoid_with_callback.called_after_destroy + assert_equal 1, @paranoid_with_callback.called_after_commit_on_destroy + end + + def test_paranoid_destroy_destroy_callbacks + @paranoid_with_callback = ParanoidWithCallback.first + ParanoidWithCallback.transaction do + @paranoid_with_callback.destroy.destroy + end + + assert_equal 2, @paranoid_with_callback.called_before_destroy + assert_equal 2, @paranoid_with_callback.called_after_destroy + assert_equal 1, @paranoid_with_callback.called_after_commit_on_destroy end def test_hard_destroy_callbacks @paranoid_with_callback = ParanoidWithCallback.first ParanoidWithCallback.transaction do - @paranoid_with_callback.destroy! + @paranoid_with_callback.destroy_fully! end - assert @paranoid_with_callback.called_before_destroy - assert @paranoid_with_callback.called_after_destroy - assert @paranoid_with_callback.called_after_commit_on_destroy + assert_equal 1, @paranoid_with_callback.called_before_destroy + assert_equal 1, @paranoid_with_callback.called_after_destroy + assert_equal 1, @paranoid_with_callback.called_after_commit_on_destroy end def test_recovery_callbacks @@ -389,22 +400,71 @@ def test_recovery_callbacks ParanoidWithCallback.transaction do @paranoid_with_callback.destroy - assert_nil @paranoid_with_callback.called_before_recover - assert_nil @paranoid_with_callback.called_after_recover + assert_equal 1, @paranoid_with_callback.called_before_destroy + assert_equal 1, @paranoid_with_callback.called_after_destroy + assert_equal 0, @paranoid_with_callback.called_after_commit_on_destroy + assert_equal 0, @paranoid_with_callback.called_before_recover + assert_equal 0, @paranoid_with_callback.called_after_recover @paranoid_with_callback.recover end - assert @paranoid_with_callback.called_before_recover - assert @paranoid_with_callback.called_after_recover + assert_equal 1, @paranoid_with_callback.called_before_destroy + assert_equal 1, @paranoid_with_callback.called_after_destroy + # whether or not the after_destroy_commit callback should be called if the record is recovered is debatable + # for now, ActiveRecord versions >= 5.2.0 will call it and the other versions will not + assert_includes 0..1, @paranoid_with_callback.called_after_commit_on_destroy + assert_equal 1, @paranoid_with_callback.called_before_recover + assert_equal 1, @paranoid_with_callback.called_after_recover + end + + def test_recovery_callbacks_with_2_transactions + @paranoid_with_callback = ParanoidWithCallback.first + + ParanoidWithCallback.transaction do + @paranoid_with_callback.destroy + end + + assert_equal 1, @paranoid_with_callback.called_before_destroy + assert_equal 1, @paranoid_with_callback.called_after_destroy + assert_equal 1, @paranoid_with_callback.called_after_commit_on_destroy + assert_equal 0, @paranoid_with_callback.called_before_recover + assert_equal 0, @paranoid_with_callback.called_after_recover + + ParanoidWithCallback.transaction do + @paranoid_with_callback.recover + end + + assert_equal 1, @paranoid_with_callback.called_before_destroy + assert_equal 1, @paranoid_with_callback.called_after_destroy + assert_equal 1, @paranoid_with_callback.called_after_commit_on_destroy + assert_equal 1, @paranoid_with_callback.called_before_recover + assert_equal 1, @paranoid_with_callback.called_after_recover + end + + def test_paranoid_destroy_with_update_callbacks + if ActiveRecord::VERSION::MAJOR < 5 || (ActiveRecord::VERSION::MAJOR == 5 && ActiveRecord::VERSION::MINOR < 1) + skip 'ActiveRecord versions < 5.1.0 cannot know in which transaction the record was destroyed' + end + @paranoid_with_callback = ParanoidWithCallback.first + ParanoidWithCallback.transaction do + @paranoid_with_callback.destroy + end + ParanoidWithCallback.transaction do + @paranoid_with_callback.update_attributes(:name => "still paranoid") + end + + assert_equal 1, @paranoid_with_callback.called_before_destroy + assert_equal 1, @paranoid_with_callback.called_after_destroy + assert_equal 1, @paranoid_with_callback.called_after_commit_on_destroy end def test_recovery_callbacks_without_destroy @paranoid_with_callback = ParanoidWithCallback.first @paranoid_with_callback.recover - assert_nil @paranoid_with_callback.called_before_recover - assert_nil @paranoid_with_callback.called_after_recover + assert_equal 0, @paranoid_with_callback.called_before_recover + assert_equal 0, @paranoid_with_callback.called_after_recover end def test_delete_by_multiple_id_is_paranoid diff --git a/test/test_helper.rb b/test/test_helper.rb index 27f9d21a..fdeaef47 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -367,29 +367,26 @@ class ParanoidWithCallback < ActiveRecord::Base before_recover :call_me_before_recover after_recover :call_me_after_recover - def initialize(*attrs) - @called_before_destroy = @called_after_destroy = @called_after_commit_on_destroy = false - super(*attrs) - end + set_callback :initialize, lambda { @called_before_destroy = @called_after_destroy = @called_after_commit_on_destroy = @called_before_recover = @called_after_recover = 0 } def call_me_before_destroy - @called_before_destroy = true + @called_before_destroy += 1 end def call_me_after_destroy - @called_after_destroy = true + @called_after_destroy += 1 end def call_me_after_commit_on_destroy - @called_after_commit_on_destroy = true + @called_after_commit_on_destroy += 1 end def call_me_before_recover - @called_before_recover = true + @called_before_recover += 1 end def call_me_after_recover - @called_after_recover = true + @called_after_recover += 1 end end