Skip to content

Commit

Permalink
Fixed bug with really_destroy! after destroy and has_ones. Refactored…
Browse files Browse the repository at this point in the history
… logic in #restore_associated_records and leverage it for really_destroy.
  • Loading branch information
harryhlai committed Nov 3, 2017
1 parent 9250578 commit fa4f990
Show file tree
Hide file tree
Showing 2 changed files with 37 additions and 33 deletions.
52 changes: 23 additions & 29 deletions lib/paranoia.rb
Original file line number Diff line number Diff line change
Expand Up @@ -141,8 +141,7 @@ def really_destroy!
end
if dependent_reflections.any?
dependent_reflections.each do |name, reflection|
association_data = self.send(name)
# has_one association can return nil
association_data = self.send(name) || get_soft_deleted_has_one(reflection)
# .paranoid? will work for both instances and classes
next unless association_data && association_data.paranoid?
if reflection.collection?
Expand Down Expand Up @@ -187,41 +186,36 @@ def restore_associated_records(recovery_window_range = nil)
end

destroyed_associations.each do |association|
association_data = send(association.name)
association_data = send(association.name) || get_soft_deleted_has_one(association)

unless association_data.nil?
if association_data.paranoid?
if association.collection?
association_data.only_deleted.each do |record|
record.restore(:recursive => true, :recovery_window_range => recovery_window_range)
end
else
association_data.restore(:recursive => true, :recovery_window_range => recovery_window_range)
if association_data && association_data.paranoid?
restore_options = { :recursive => true, :recovery_window_range => recovery_window_range }
if association.collection?
association_data.only_deleted.each do |record|
record.restore(restore_options)
end
end
end

if association_data.nil? && association.macro.to_s == "has_one"
association_class_name = association.klass.name
association_foreign_key = association.foreign_key

if association.type
association_polymorphic_type = association.type
association_find_conditions = { association_polymorphic_type => self.class.name.to_s, association_foreign_key => self.id }
else
association_find_conditions = { association_foreign_key => self.id }
end

association_class = association_class_name.constantize
if association_class.paranoid?
association_class.only_deleted.where(association_find_conditions).first
.try!(:restore, recursive: true, :recovery_window_range => recovery_window_range)
association_data.restore(restore_options)
end
end
end

clear_association_cache if destroyed_associations.present?
end

# For soft deleted objects, has_one associations will return nil if the
# associated object is also soft deleted. Because of this, we have to do the
# object look-up explicitly. This method takes an association, and if it is
# a has_one and the object referenced by the assocation uses paranoia, it
# returns the object referenced by the association. Otherwise, it returns nil.
def get_soft_deleted_has_one(association)
return nil unless association.macro.to_s == "has_one"

association_find_conditions = { association.foreign_key => self.id }
association_find_conditions[association.type] = self.class.name if association.type

association.klass.only_deleted.find_by(association_find_conditions) if association.klass.paranoid?
end
end

ActiveSupport.on_load(:active_record) do
Expand Down Expand Up @@ -301,7 +295,7 @@ def build_relation(klass, *args)
class UniquenessValidator < ActiveModel::EachValidator
prepend UniquenessParanoiaValidator
end

class AssociationNotSoftDestroyedValidator < ActiveModel::EachValidator
def validate_each(record, attribute, value)
# if association is soft destroyed, add an error
Expand Down
18 changes: 14 additions & 4 deletions test/paranoia_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ def setup!
'active_column_models' => 'deleted_at DATETIME, active BOOLEAN',
'active_column_model_with_uniqueness_validations' => 'name VARCHAR(32), deleted_at DATETIME, active BOOLEAN',
'paranoid_model_with_belongs_to_active_column_model_with_has_many_relationships' => 'name VARCHAR(32), deleted_at DATETIME, active BOOLEAN, active_column_model_with_has_many_relationship_id INTEGER',
'active_column_model_with_has_many_relationships' => 'name VARCHAR(32), deleted_at DATETIME, active BOOLEAN',
'active_column_model_with_has_many_relationships' => 'name VARCHAR(32), deleted_at DATETIME, active BOOLEAN',
'without_default_scope_models' => 'deleted_at DATETIME'
}.each do |table_name, columns_as_sql_string|
ActiveRecord::Base.connection.execute "CREATE TABLE #{table_name} (id INTEGER NOT NULL PRIMARY KEY, #{columns_as_sql_string})"
Expand Down Expand Up @@ -189,11 +189,11 @@ def test_scoping_behavior_for_paranoid_models
p2 = ParanoidModel.create(:parent_model => parent2)
p1.destroy
p2.destroy

assert_equal 0, parent1.paranoid_models.count
assert_equal 1, parent1.paranoid_models.only_deleted.count

assert_equal 2, ParanoidModel.only_deleted.joins(:parent_model).count
assert_equal 2, ParanoidModel.only_deleted.joins(:parent_model).count
assert_equal 1, parent1.paranoid_models.deleted.count
assert_equal 0, parent1.paranoid_models.without_deleted.count
p3 = ParanoidModel.create(:parent_model => parent1)
Expand All @@ -206,7 +206,7 @@ def test_only_deleted_with_joins
c1 = ActiveColumnModelWithHasManyRelationship.create(name: 'Jacky')
c2 = ActiveColumnModelWithHasManyRelationship.create(name: 'Thomas')
p1 = ParanoidModelWithBelongsToActiveColumnModelWithHasManyRelationship.create(name: 'Hello', active_column_model_with_has_many_relationship: c1)

c1.destroy
assert_equal 1, ActiveColumnModelWithHasManyRelationship.count
assert_equal 1, ActiveColumnModelWithHasManyRelationship.only_deleted.count
Expand Down Expand Up @@ -531,6 +531,15 @@ def test_real_destroy_dependent_destroy_after_normal_destroy
refute RelatedModel.unscoped.exists?(child.id)
end

def test_real_destroy_with_has_one_dependent_destroy_after_normal_destroy
parent = ParentModel.create
child = parent.paranoid_model = ParanoidModel.create
parent.destroy
parent.reload
parent.really_destroy!
refute ParanoidModel.unscoped.exists?(child.id)
end

def test_real_destroy_dependent_destroy_after_normal_destroy_does_not_delete_other_children
parent_1 = ParentModel.create
child_1 = parent_1.very_related_models.create
Expand Down Expand Up @@ -1130,6 +1139,7 @@ class ParentModel < ActiveRecord::Base
has_one :non_paranoid_model, dependent: :destroy
has_many :asplode_models, dependent: :destroy
has_one :polymorphic_model, as: :parent, dependent: :destroy
has_one :paranoid_model, dependent: :destroy
end

class ParentModelWithCounterCacheColumn < ActiveRecord::Base
Expand Down

0 comments on commit fa4f990

Please sign in to comment.