From 9cd15939c1b59f2cd59ba3196946b486f1db9052 Mon Sep 17 00:00:00 2001 From: Andy Klimczak Date: Sun, 10 Jun 2018 13:15:42 -0400 Subject: [PATCH] Fix `SystemStackError` when paranoid models have a circular `dependent: :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 --- .travis.yml | 4 +++- lib/paranoia.rb | 5 +---- lib/paranoia/active_record_5_2.rb | 6 ++++++ test/paranoia_test.rb | 28 ++++++++++++++++++++++++++-- 4 files changed, 36 insertions(+), 7 deletions(-) diff --git a/.travis.yml b/.travis.yml index 78b46b95..4febcaec 100644 --- a/.travis.yml +++ b/.travis.yml @@ -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 diff --git a/lib/paranoia.rb b/lib/paranoia.rb index c1b323fc..ac8b6f0f 100644 --- a/lib/paranoia.rb +++ b/lib/paranoia.rb @@ -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 diff --git a/lib/paranoia/active_record_5_2.rb b/lib/paranoia/active_record_5_2.rb index c9c5463f..f28202d5 100644 --- a/lib/paranoia/active_record_5_2.rb +++ b/lib/paranoia/active_record_5_2.rb @@ -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? @@ -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? diff --git a/test/paranoia_test.rb b/test/paranoia_test.rb index e08f9a7b..5e1d3c73 100644 --- a/test/paranoia_test.rb +++ b/test/paranoia_test.rb @@ -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 @@ -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 @@ -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_"