Skip to content

Commit

Permalink
Fix SystemStackError when paranoid models have a circular `dependen…
Browse files Browse the repository at this point in the history
…t: :destroy`

- Previously, paranoid models with a circular `dependent: :destroy` would recursively try to delete eachother
- Add a variable to check if the destroy callback has already been
called for that model
- Similar fix to rails/rails#18548
  • Loading branch information
andyklimczak committed Sep 13, 2019
1 parent 0600183 commit 28a8d64
Show file tree
Hide file tree
Showing 4 changed files with 36 additions and 7 deletions.
4 changes: 3 additions & 1 deletion .travis.yml
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
sudo: false
language: ruby
before_install: gem update --system
before_install:
- gem uninstall -v '>= 2' -i $(rvm gemdir)@global -ax bundler || true
- gem install bundler -v '< 2'
cache: bundler
rvm:
- 2.2
Expand Down
5 changes: 1 addition & 4 deletions lib/paranoia.rb
Original file line number Diff line number Diff line change
@@ -1,9 +1,6 @@
require 'active_record' unless defined? ActiveRecord

if [ActiveRecord::VERSION::MAJOR, ActiveRecord::VERSION::MINOR] == [5, 2] ||
ActiveRecord::VERSION::MAJOR > 5
require 'paranoia/active_record_5_2'
end
require 'paranoia/active_record_5_2'

module Paranoia
@@default_sentinel_value = nil
Expand Down
6 changes: 6 additions & 0 deletions lib/paranoia/active_record_5_2.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@ def handle_dependency

case options[:dependent]
when :destroy
@_destroy_callback_already_called ||= false
return if @_destroy_callback_already_called
@_destroy_callback_already_called = true
target.destroy
if target.respond_to?(:paranoia_destroyed?)
raise ActiveRecord::Rollback unless target.paranoia_destroyed?
Expand All @@ -23,7 +26,10 @@ def delete(method = options[:dependent])
when :delete
target.delete
when :destroy
@_destroy_callback_already_called ||= false
return if @_destroy_callback_already_called
target.destroyed_by_association = reflection
@_destroy_callback_already_called = true
target.destroy
if target.respond_to?(:paranoia_destroyed?)
throw(:abort) unless target.paranoia_destroyed?
Expand Down
28 changes: 26 additions & 2 deletions test/paranoia_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,10 @@ 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',
'without_default_scope_models' => 'deleted_at DATETIME'
'active_column_model_with_has_many_relationships' => 'name VARCHAR(32), deleted_at DATETIME, active BOOLEAN',
'without_default_scope_models' => 'deleted_at DATETIME',
'paranoid_has_one_dependent_destroys' => 'deleted_at DATETIME',
'paranoid_belongs_to_dependent_destroys' => 'deleted_at DATETIME, paranoid_has_one_dependent_destroy_id INTEGER'
}.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})"
end
Expand Down Expand Up @@ -1053,6 +1055,18 @@ def test_counter_cache_column_on_restore
related_model.restore
assert_equal 1, parent_model_with_counter_cache_column.reload.related_models_count
end

def test_circular_dependent_destroy_has_one_belongs_to
has_one_dependent_destroy = ParanoidHasOneDependentDestroy.create
belongs_to_dependent_destroy = ParanoidBelongsToDependentDestroy.create
belongs_to_dependent_destroy.paranoid_has_one_dependent_destroy = has_one_dependent_destroy
belongs_to_dependent_destroy.save!

belongs_to_dependent_destroy.destroy

refute has_one_dependent_destroy.reload.deleted_at.nil?
refute belongs_to_dependent_destroy.reload.deleted_at.nil?
end
end

private
Expand Down Expand Up @@ -1356,6 +1370,16 @@ class PolymorphicModel < ActiveRecord::Base
belongs_to :parent, polymorphic: true
end

class ParanoidHasOneDependentDestroy < ActiveRecord::Base
acts_as_paranoid
has_one :paranoid_belongs_to_dependent_destroy, dependent: :destroy
end

class ParanoidBelongsToDependentDestroy < ActiveRecord::Base
acts_as_paranoid
belongs_to :paranoid_has_one_dependent_destroy, dependent: :destroy
end

module Namespaced
def self.table_name_prefix
"namespaced_"
Expand Down

0 comments on commit 28a8d64

Please sign in to comment.