Skip to content

Fix really_destroy used against a has_one relationship that is soft deleted #574

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.

Sign up for GitHub

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

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

Fix really_destroy used against a has_one relationship that is soft deleted #574

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ce28b2e
Fix really_destroy used against a has_one relationship that is soft d…
Feb 6, 2025
16ebd82
fix: make previous fix work for polymorphic relationships
Mar 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions lib/paranoia.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,11 @@ def really_destroy!(update_destroy_attributes: true)
association_data = self.send(name)
# has_one association can return nil
# .paranoid? will work for both instances and classes
if association_data.nil? && reflection.has_one? && reflection.klass.paranoid?
query = { reflection.foreign_key => self.id }
query[reflection.type] = self.class.name if reflection.options[:as] # Handle polymorphic associations
association_data = reflection.klass.only_deleted.find_by(query)
end
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yoheimuta Can you chime in about this change?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mathieujobin Looks good to me! 👍

[nits] For has_one associations that are polymorphic (foreign_type column exists), find_by should also filter by foreign_type to avoid fetching unrelated records.

Suggested improvement:

query = { reflection.foreign_key => self.id }
query[reflection.type] = self.class.name if reflection.options[:as] # Handle polymorphic associations
association_data = reflection.klass.only_deleted.find_by(query)

This ensures we correctly match records when dealing with polymorphic has_one associations.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@reedstonefood Can you review this suggestion, see if that matches with your use case, etc.

thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change works with the test suite here & in the product I'm working on, so LGTM.

I've also managed to add another test that fails without this change in place 👍 .

next unless association_data && association_data.paranoid?
if reflection.collection?
next association_data.with_deleted.find_each { |record|
Expand Down
24 changes: 24 additions & 0 deletions test/paranoia_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1419,6 +1419,30 @@ def test_update_has_many_through_relation_delete_associations
assert_equal 2, employer.jobs.with_deleted.count
end

def test_really_destroy_against_soft_deleted_object_with_has_one_association
model = ParanoidModelWithHasOne.create(paranoid_model_with_belong: ParanoidModelWithBelong.create)
assert_equal 1, ParanoidModelWithBelong.with_deleted.count
model.destroy
assert_equal 1, ParanoidModelWithBelong.with_deleted.count
model.reload
model.really_destroy!
assert_equal 0, ParanoidModelWithBelong.with_deleted.count
end

def test_really_destroy_against_soft_deleted_object_with_polymorphic_has_one_association
unrelated_model = PolymorphicModel.create(parent_id: 1, parent_type: 'AModel')
model = ParentModel.create(id: 1, polymorphic_model: PolymorphicModel.create)
unrelated_model.destroy # means that later on in the test, there will be 2 deleted polymorphic models with parent_id of 1

assert_equal 2, PolymorphicModel.with_deleted.count
model.destroy
assert_equal 2, PolymorphicModel.with_deleted.count
model.reload
model.really_destroy!
assert_equal 1, PolymorphicModel.with_deleted.count
assert_equal 'AModel', PolymorphicModel.with_deleted.first.parent_type
end

private
def get_featureful_model
FeaturefulModel.new(:name => "not empty")
Expand Down