Add support for bidirectional destroy dependencies

Prior to this commit if you defined a bidirectional relationship
between two models with destroy dependencies on both sides, a call to
`destroy` would result in an infinite callback loop.

Take the following relationship.

    class Content < ActiveRecord::Base
      has_one :content_position, dependent: :destroy
    end

    class ContentPosition < ActiveRecord::Base
      belongs_to :content, dependent: :destroy
    end

Calling `Content#destroy` or `ContentPosition#destroy` would result in
an infinite callback loop.

This commit changes the behaviour of `ActiveRecord::Callbacks#destroy`
so that it guards against subsequent callbacks.

Thanks to @zetter for demonstrating the issue with failing tests[1].

[1] rails#13609

7 files changed, 119 insertions, 1 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index c777af342f..d0dcff801b 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,19 @@
+*   Add support for bidirectional destroy dependencies.
+
+    Fixes #13609.
+
+    Example:
+
+        class Content < ActiveRecord::Base
+          has_one :position, dependent: :destroy
+        end
+
+        class Position < ActiveRecord::Base
+          belongs_to :content, dependent: :destroy
+        end
+
+    *Seb Jacobs*
+
 *   `time` columns can now affected by `time_zone_aware_attributes`. If you have
     set `config.time_zone` to a value other than `'UTC'`, they will be treated
     as in that time zone by default in Rails 5.0. If this is not the desired
diff --git a/activerecord/lib/active_record/callbacks.rb b/activerecord/lib/active_record/callbacks.rb
index f44e5af5de..07769e8952 100644
--- a/activerecord/lib/active_record/callbacks.rb
+++ b/activerecord/lib/active_record/callbacks.rb
@@ -289,7 +289,14 @@ module ActiveRecord
     end
 
     def destroy #:nodoc:
-      _run_destroy_callbacks { super }
+      begin
+        @_destroy_callback_already_called ||= false
+        return if @_destroy_callback_already_called
+        @_destroy_callback_already_called = true
+        _run_destroy_callbacks { super }
+      ensure
+        @_destroy_callback_already_called = false
+      end
     end
 
     def touch(*) #:nodoc:
diff --git a/activerecord/test/cases/associations/bidirectional_destroy_dependencies_test.rb b/activerecord/test/cases/associations/bidirectional_destroy_dependencies_test.rb
new file mode 100644
index 0000000000..2b867965ba
--- /dev/null
+++ b/activerecord/test/cases/associations/bidirectional_destroy_dependencies_test.rb
@@ -0,0 +1,41 @@
+require 'cases/helper'
+require 'models/content'
+
+class BidirectionalDestroyDependenciesTest < ActiveRecord::TestCase
+  fixtures :content, :content_positions
+
+  def setup
+    Content.destroyed_ids.clear
+    ContentPosition.destroyed_ids.clear
+  end
+
+  def test_bidirectional_dependence_when_destroying_item_with_belongs_to_association
+    content_position = ContentPosition.find(1)
+    content = content_position.content
+    assert_not_nil content
+
+    content_position.destroy
+
+    assert_equal [content_position.id], ContentPosition.destroyed_ids
+    assert_equal [content.id], Content.destroyed_ids
+  end
+
+  def test_bidirectional_dependence_when_destroying_item_with_has_one_association
+    content = Content.find(1)
+    content_position = content.content_position
+    assert_not_nil content_position
+
+    content.destroy
+
+    assert_equal [content.id], Content.destroyed_ids
+    assert_equal [content_position.id], ContentPosition.destroyed_ids
+  end
+
+  def test_bidirectional_dependence_when_destroying_item_with_has_one_association_fails_first_time
+    content = ContentWhichRequiresTwoDestroyCalls.find(1)
+
+    2.times { content.destroy }
+
+    assert_equal content.destroyed?, true
+  end
+end
diff --git a/activerecord/test/fixtures/content.yml b/activerecord/test/fixtures/content.yml
new file mode 100644
index 0000000000..0d12ee03dc
--- /dev/null
+++ b/activerecord/test/fixtures/content.yml
@@ -0,0 +1,3 @@
+content:
+  id: 1
+  title: How to use Rails
diff --git a/activerecord/test/fixtures/content_positions.yml b/activerecord/test/fixtures/content_positions.yml
new file mode 100644
index 0000000000..9e85773f8e
--- /dev/null
+++ b/activerecord/test/fixtures/content_positions.yml
@@ -0,0 +1,3 @@
+content_positions:
+  id: 1
+  content_id: 1
diff --git a/activerecord/test/models/content.rb b/activerecord/test/models/content.rb
new file mode 100644
index 0000000000..140e1dfc78
--- /dev/null
+++ b/activerecord/test/models/content.rb
@@ -0,0 +1,40 @@
+class Content < ActiveRecord::Base
+  self.table_name = 'content'
+  has_one :content_position, dependent: :destroy
+
+  def self.destroyed_ids
+    @destroyed_ids ||= []
+  end
+
+  before_destroy do |object|
+    Content.destroyed_ids << object.id
+  end
+end
+
+class ContentWhichRequiresTwoDestroyCalls < ActiveRecord::Base
+  self.table_name = 'content'
+  has_one :content_position, foreign_key: 'content_id', dependent: :destroy
+
+  after_initialize do
+    @destroy_count = 0
+  end
+
+  before_destroy do
+    @destroy_count += 1
+    if @destroy_count == 1
+      throw :abort
+    end
+  end
+end
+
+class ContentPosition < ActiveRecord::Base
+  belongs_to :content, dependent: :destroy
+
+  def self.destroyed_ids
+    @destroyed_ids ||= []
+  end
+
+  before_destroy do |object|
+    ContentPosition.destroyed_ids << object.id
+  end
+end
diff --git a/activerecord/test/schema/schema.rb b/activerecord/test/schema/schema.rb
index 21b23d8e0c..477afe6feb 100644
--- a/activerecord/test/schema/schema.rb
+++ b/activerecord/test/schema/schema.rb
@@ -216,6 +216,14 @@ ActiveRecord::Schema.define do
   add_index :companies, [:firm_id, :type], name: "company_partial_index", where: "rating > 10"
   add_index :companies, :name, name: 'company_name_index', using: :btree
 
+  create_table :content, force: true do |t|
+    t.string :title
+  end
+
+  create_table :content_positions, force: true do |t|
+    t.integer :content_id
+  end
+
   create_table :vegetables, force: true do |t|
     t.string :name
     t.integer :seller_id