Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for recursive delete all associations #326

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ryan-secondsight
Copy link

@ryan-secondsight ryan-secondsight commented Apr 11, 2024

When having nested associations that use delete_all, the grandchildren of the associations do not get deleted. This can result in some orphaned records.

This PR adds an optional configuration to automatically cascade delete dependent relationships where the top level relationship is a delete_all.

For instance:

class ParanoidParent < ActiveRecord::Base
  acts_as_paranoid(handle_delete_all_associations: true)

  has_many :paranoid_children, dependent: :delete_all
end

class ParanoidChild < ActiveRecord::Base
  acts_as_paranoid(handle_delete_all_associations: true)

  belongs_to :paranoid_parent
  has_many :paranoid_grandchildren, dependent: :delete_all
end

class ParanoidGrandchild < ActiveRecord::Base
  acts_as_paranoid(handle_delete_all_associations: true)

  belongs_to :paranoid_child
end

When calling parent.destroy, the Grandchildren are not deleted. This PR would cascade the delete_all through to the children. I have found this behavior helpful when there are thousands of parent.children, and each of those children have grandchildren.

This PR also handles the recovery of the associations as well.

If this idea gets approved, I can clean up the Rubocop violations and flesh out any tests, if required.

@mvz
Copy link
Contributor

mvz commented Apr 14, 2024

Hi @ryan-secondsight, I don't think standard Rails deletes the grandchildren in this case. At least, if I remove the paranoid configuration from your test file the grandchildren are not deleted. Therefore, it wouldn't make sense for acts_as_paranoid to do so. If you want to cascade the removal of the parent to the grandchildren, you should probably use dependent: :destroy.

@ryan-secondsight
Copy link
Author

Fair enough -- vanilla delete_all doesn't fire any callbacks so it's not typically expected.

There are too many records to do a dependent: :destroy (it would take several minutes, whereas this takes about a second), though maybe dependent: :destroy_async would work. That would require another acts_as_paranoid patch.

Thank you for the response.

@ryan-secondsight
Copy link
Author

Hello @mvz -- thank you again for your consideration in this PR.

I have been reflecting on your comments about standard Rails delete_all on grandchildren, and I found one way that Rails allows for this behavior natively: by updating the migration to use references and passing the correct configuration to enable cascading deletes, upon deleting the Parent, all of the Children and Grandchildren are also deleted.

I have updated the PR's migrations to include this behavior -- if you are to remove paranoid from the models and delete the parent, you can see that the children and grandchildren are also deleted.

I believe this feature will be helpful in cases where it is not feasible to use dependent: :destroy, such as where cascading deletes have been used to hard-delete hundreds of thousands of records. I am including this functionality as a flag that must be enabled separately, as to not alter the default behavior of acts_as_paranoid.

Given this, would you be open to reconsidering accepting the merge request?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants