Issues with callbacks (infinite loops, record status...) #108

aymeric-ledorze · 2018-08-01T15:07:16Z

There are various issues when dealing with callbacks when destroying a record:

A record is dirty after being destroyed. and Rails therefore considers that there are still changes to be saved to the database. Thus, if an after_destroy callback modifies the deleted record again, this may trigger new unwanted callbacks. Moreover, this prevents a record to be recovered after being destroyed without reloading it.
There is no way to know if a record was soft deleted or hard deleted in a destroy callback.
The after_destroy_commit callbacks are not called when hard destroying a record.
If a record is destroyed and then modified, the after_destroy_commit callbacks are called twice. This is what triggers issue SystemStackError during destroy with ActiveStorage 5.2 #103.

This PR solves all these problems with the following changes:

The changes on the paranoid_value are removed when destroying a record.
The instance variable @destroyed is set to true only when the record is removed from the database, ie in destroy_fully!. This is how Rails manages destruction and knows that a record is no longer persisted. Thus, deleted? indicates if a record is deleted (soft or hard) and deleted_fully? indicates if a record is hard deleted.
The instance variable @_trigger_destroy_callback is now also defined when hard destroying a record.
The @_trigger_destroy_callback variable is set back to nil after soft destroying a record at the end of a transaction or when a new transaction starts. This is how Rails solved a similar issue when creating a record. This means that if a record is destroyed and updated in the same transaction, the after_destroy_callbacks are correctly called once only with the very last versions of Rails (5.2.1.rc1 as of now). Note that for Rails versions older than 5.1, the callbacks are still incorrectly called twice even with two separate transactions.

This PR also adds a test with the destruction of a record with ActiveStorage attachments. The loading of ActiveStorage is kinda complicated and there might be room for improvement. The previous change fixes the SystemStackError described by issue #103 but does not prevents the destruction of attachments when soft destroying a record.

There are still open issues. For example, there is still no way to know if a record is expected to be soft or hard deleted in a before_destroy callback. There is also an undefined behavior when destroying and recovering a record in the same transaction. Should after_destroy_commit be called? It currently depends on the Rails version...

auscaster · 2019-01-02T17:48:22Z

It would be really great to get this merged.

mvz

Hi @aymeric-ledorze, this looks good overall. I have just a few small comments.

Can you please rebase this on current master?

mvz · 2019-09-15T11:49:55Z

lib/acts_as_paranoid/core.rb

@@ -2,6 +2,12 @@ module ActsAsParanoid
  module Core
    def self.included(base)
      base.extend ClassMethods
+      if ActiveRecord::VERSION::MAJOR > 5 || (ActiveRecord::VERSION::MAJOR == 5 && (ActiveRecord::VERSION::MINOR >= 2 || (ActiveRecord::VERSION::MINOR == 2 && ActiveRecord::VERSION::TINY >= 1)))


I'm not sure what (ActiveRecord::VERSION::MINOR == 2 && ActiveRecord::VERSION::TINY >= 1) is supposed to do, because if the minor version is 2, then the previous check (ActiveRecord::VERSION::MINOR >= 2) is also already true.

This looks like a mistake. I wanted to run this code only with rails versions >= 5.2.1 but I forgot to add a strict comparison in the minor part.

mvz · 2019-09-15T11:52:32Z

lib/acts_as_paranoid/core.rb

@@ -199,7 +210,7 @@ def destroy_dependent_associations!
    end

    def deleted?
-      !if self.class.string_type_with_deleted_value?
+      @destroyed || !if self.class.string_type_with_deleted_value?


The logic here is already quite complex and in need of a clean-up. How about moving the @destroyed check to a separate line, like:

return true if @destroyed

No problem, but it seems that you already integrated it that way. There is no problem if you want to change it again.

mvz · 2019-09-15T11:53:39Z

lib/acts_as_paranoid/core.rb

@@ -223,5 +240,28 @@ def get_reflection_class(reflection)
    def paranoid_value=(value)
      self.send("#{self.class.paranoid_column}=", value)
    end
+
+    def stale_paranoid_value


This name is confusing. How about mark_as_soft_deleted or something like that?

Yes, why not? I agree that the method name is confusing but the idea was that the record is actually already marked as soft deleted in the database and the goal of this method is to sync the loaded record with the database state.

mvz · 2019-09-15T11:55:38Z

test/test_core.rb

+    assert_not ParanoidTime.with_deleted.first.deleted_fully?
+
+    ParanoidString.first.destroy
+    assert ParanoidString.with_deleted.first.deleted?


Shouldn't this be an assertion about #deleted_fully??

Maybe. I'm sorry, I can't remember why I wrote that test this way... I think we could both test deleted? and not deleted_fully? for the two paranoid objects.

mvz · 2019-09-15T12:33:39Z

lib/acts_as_paranoid/core.rb

+        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
+      base.send(:alias_method, :force_clear_transaction_record_state_without_paranoid, :force_clear_transaction_record_state)


You can avoid the base.send ... by using class << base, as done in lib/acts_as_paranoid/associations.rb

It is not that simple because in your example, you redefined belongs_to which is a class method and here we are dealing with instance methods. We could use a class_eval however.

mvz · 2019-10-22T10:32:55Z

Since this a rather large set of changes, I'll be merging them in one by one.

aymeric-ledorze · 2019-10-28T11:41:12Z

Sorry I did not respond earlier but I am rather busy at the moment. I would like to thank you for accepting this commit !
I rebased my branch on master and the only real issue I had was with some change with ActiveRecord 6.0 where every record is now synced with the transaction state every time we read any attribute. Since we need to read "deleted_at" when syncing with the transaction state to remove _trigger_destroy_callback, we were stuck in an infinite loop. I fixed this issued by directly retrieving the "deleted_at" value using @attributes.fetch since this is how Rails did it to retrieve the id in a similar situation. However, due to the complex logic of the "deleted?" method, I added an optional argument to directly pass the attribute value rather than reading the column, which would trigger the said infinite loop.

aymeric-ledorze · 2020-11-20T18:16:13Z

I just noticed that the test about callbacks that I added failed with newer Rails versions. Actually, it failed only with Rails > 6.0.0 since Rails fixed issues about commit callbacks starting from version 6.0.1 (PR 37251). This PR fixes a lot of issues and therefore makes most of my changes completely useless with newer Rails versions. However, they are still needed with older versions. This PR is also merged in Rails 5.2, starting with version 5.2.4.
So I updated my code to take theses changes into account. The only change still needed for Rails 6 is the setting of @_trigger_destroy_callback in destroy_fully! This means that the dirty hack that I added to the deleted? method with its optional parameter is no longer needed. Note that I decided to not support Rails 6.0.0 and other Rails versions prior to 5.2.4.

As for the ActiveStorage test, Rails changed ActiveStorage loading so I had to add a "fake" Rails module to prevent a crash. But I also noticed that it will change again in future Rails version so I prefer to wait for the stabilisation of the Rails 6.1 branch before fixing it for the active_record_edge gemfile.

…g called too many times.

mvz reviewed Sep 15, 2019

View reviewed changes

mvz added the more info needed label Oct 22, 2019

This was referenced Oct 22, 2019

Fix a record being dirty after destroying it #135

Merged

Add predicate to check if record was soft deleted or hard deleted #136

Merged

aymeric-ledorze force-pushed the master branch from 7c34449 to 9434f32 Compare October 28, 2019 11:38

aymeric-ledorze force-pushed the master branch 2 times, most recently from eef44d3 to d81cdd8 Compare November 20, 2020 17:49

aymeric-ledorze force-pushed the master branch from d81cdd8 to e0aa8ce Compare November 23, 2020 10:55

mvz mentioned this pull request Jun 5, 2021

No need to stale paranoid_value when destroying fully #238

Merged

aymeric-ledorze force-pushed the master branch 5 times, most recently from cc61e44 to ac1df78 Compare October 27, 2021 17:41

aymeric-ledorze force-pushed the master branch 2 times, most recently from 6d8d43c to a32759d Compare April 13, 2022 07:24

mvz removed the more info needed label May 1, 2022

aymeric-ledorze force-pushed the master branch from a32759d to d2cca1d Compare July 4, 2023 08:56

aymeric-ledorze force-pushed the master branch from d2cca1d to b7783d0 Compare October 10, 2023 11:50

aymeric-ledorze added 2 commits October 10, 2023 13:52

Fix many callback problems such as callbacks not being called or bein…

a2a6a83

…g called too many times.

Added a test with active_storage (issue ActsAsParanoid#103)

735fef5

aymeric-ledorze force-pushed the master branch from b7783d0 to 735fef5 Compare October 10, 2023 11:57

aymeric-ledorze mentioned this pull request Oct 10, 2023

SystemStackError during destroy with ActiveStorage 5.2 #103

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issues with callbacks (infinite loops, record status...) #108

Issues with callbacks (infinite loops, record status...) #108

aymeric-ledorze commented Aug 1, 2018

auscaster commented Jan 2, 2019

mvz left a comment

mvz Sep 15, 2019 •

edited

Loading

aymeric-ledorze Oct 28, 2019

mvz Sep 15, 2019

aymeric-ledorze Oct 28, 2019

mvz Sep 15, 2019

aymeric-ledorze Oct 28, 2019

mvz Sep 15, 2019

aymeric-ledorze Oct 28, 2019

mvz Sep 15, 2019

aymeric-ledorze Oct 28, 2019

mvz commented Oct 22, 2019

aymeric-ledorze commented Oct 28, 2019 •

edited

Loading

aymeric-ledorze commented Nov 20, 2020 •

edited

Loading

Issues with callbacks (infinite loops, record status...) #108

Are you sure you want to change the base?

Issues with callbacks (infinite loops, record status...) #108

Conversation

aymeric-ledorze commented Aug 1, 2018

auscaster commented Jan 2, 2019

mvz left a comment

Choose a reason for hiding this comment

mvz Sep 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvz commented Oct 22, 2019

aymeric-ledorze commented Oct 28, 2019 • edited Loading

aymeric-ledorze commented Nov 20, 2020 • edited Loading

mvz Sep 15, 2019 •

edited

Loading

aymeric-ledorze commented Oct 28, 2019 •

edited

Loading

aymeric-ledorze commented Nov 20, 2020 •

edited

Loading