From 19b168e611fb8fb547981c4535130c29856efd3a Mon Sep 17 00:00:00 2001
From: Agis- <corestudiosinc@gmail.com>
Date: Mon, 24 Aug 2015 11:27:06 +0300
Subject: [PATCH] Only nullify persisted has_one target associations

Since after 87d1aba3c `dependent: :destroy` callbacks on has_one
assocations run *after* destroy, it is possible that a nullification is
attempted on an already destroyed target:

    class Car < ActiveRecord::Base
      has_one :engine, dependent: :nullify
    end

    class Engine < ActiveRecord::Base
      belongs_to :car, dependent: :destroy
    end

    > car = Car.create!
    > engine = Engine.create!(car: car)
    > engine.destroy! # => ActiveRecord::ActiveRecordError: cannot update a
    >   destroyed record

In the above case, `engine.destroy!` deletes `engine` and *then* triggers the
deletion of `car`, which in turn triggers a nullification of `engine.car_id`.
However, `engine` is already destroyed at that point.

Fixes #21223.
---
 activerecord/CHANGELOG.md                                 | 6 ++++++
 .../lib/active_record/associations/has_one_association.rb | 2 +-
 .../test/cases/associations/has_one_associations_test.rb  | 8 ++++++++
 activerecord/test/models/developer.rb                     | 1 +
 activerecord/test/models/ship.rb                          | 1 +
 activerecord/test/schema/schema.rb                        | 1 +
 6 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 0841b2f679..d880470d97 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,9 @@
+*   Only try to nullify has_one target association if the record is persisted.
+
+    Fixes #21223.
+
+    *Agis Anastasopoulos*
+
 *   Uniqueness validator raises descriptive error when running on a persisted
     record without primary key.
 
diff --git a/activerecord/lib/active_record/associations/has_one_association.rb b/activerecord/lib/active_record/associations/has_one_association.rb
index 5a92bc5e8a..1829453d73 100644
--- a/activerecord/lib/active_record/associations/has_one_association.rb
+++ b/activerecord/lib/active_record/associations/has_one_association.rb
@@ -65,7 +65,7 @@ def delete(method = options[:dependent])
             when :destroy
               target.destroy
             when :nullify
-              target.update_columns(reflection.foreign_key => nil)
+              target.update_columns(reflection.foreign_key => nil) if target.persisted?
           end
         end
       end
diff --git a/activerecord/test/cases/associations/has_one_associations_test.rb b/activerecord/test/cases/associations/has_one_associations_test.rb
index 5a8afaf4d2..d46e7ad235 100644
--- a/activerecord/test/cases/associations/has_one_associations_test.rb
+++ b/activerecord/test/cases/associations/has_one_associations_test.rb
@@ -107,6 +107,14 @@ def test_nullification_on_association_change
     assert_nil Account.find(old_account_id).firm_id
   end
 
+  def test_nullification_on_destroyed_association
+    developer = Developer.create!(name: "Someone")
+    ship = Ship.create!(name: "Planet Caravan", developer: developer)
+    ship.destroy
+    assert !ship.persisted?
+    assert !developer.persisted?
+  end
+
   def test_natural_assignment_to_nil_after_destroy
     firm = companies(:rails_core)
     old_account_id = firm.account.id
diff --git a/activerecord/test/models/developer.rb b/activerecord/test/models/developer.rb
index d2a5a7fc49..8ac7a9df6a 100644
--- a/activerecord/test/models/developer.rb
+++ b/activerecord/test/models/developer.rb
@@ -50,6 +50,7 @@ def find_least_recent
   has_many :firms, :through => :contracts, :source => :firm
   has_many :comments, ->(developer) { where(body: "I'm #{developer.name}") }
   has_many :ratings, through: :comments
+  has_one :ship, dependent: :nullify
 
   scope :jamises, -> { where(:name => 'Jamis') }
 
diff --git a/activerecord/test/models/ship.rb b/activerecord/test/models/ship.rb
index 95172e4d3e..e333b964ab 100644
--- a/activerecord/test/models/ship.rb
+++ b/activerecord/test/models/ship.rb
@@ -3,6 +3,7 @@ class Ship < ActiveRecord::Base
 
   belongs_to :pirate
   belongs_to :update_only_pirate, :class_name => 'Pirate'
+  belongs_to :developer, dependent: :destroy
   has_many :parts, :class_name => 'ShipPart'
   has_many :treasures
 
diff --git a/activerecord/test/schema/schema.rb b/activerecord/test/schema/schema.rb
index 6f34115534..994ece9244 100644
--- a/activerecord/test/schema/schema.rb
+++ b/activerecord/test/schema/schema.rb
@@ -673,6 +673,7 @@ def except(adapter_names_to_exclude)
   create_table :ships, force: true do |t|
     t.string :name
     t.integer :pirate_id
+    t.belongs_to :developer
     t.integer :update_only_pirate_id
     # Conventionally named column for counter_cache
     t.integer :treasures_count, default: 0
-- 
GitLab