From 3da0024db472b5697d4b6f17a9e4edefdf4d6a96 Mon Sep 17 00:00:00 2001
From: Sergiy Kukunin <sergey.kukunin@gmail.com>
Date: Wed, 20 Mar 2019 12:04:39 +0200
Subject: Fix unintended autosave on has_one through association

Fixes #35680

The problem occurred, when a `has one through` association contains
a foreign key (it belongs to the intermediate association).

For example, Comment belongs to Post, Post belongs to Author, and Author
`has_one :first_comment, through: :first_post`.

In this case, the value of the foreign key is comparing with the original
record, and since they are likely different, the association is marked
as changed. So it updates every time when the origin record updates.
---
 .../lib/active_record/autosave_association.rb      |  8 ++++-
 .../test/cases/autosave_association_test.rb        | 37 ++++++++++++++++++----
 2 files changed, 38 insertions(+), 7 deletions(-)

(limited to 'activerecord')

diff --git a/activerecord/lib/active_record/autosave_association.rb b/activerecord/lib/active_record/autosave_association.rb
index d77d76cb1e..fe94662543 100644
--- a/activerecord/lib/active_record/autosave_association.rb
+++ b/activerecord/lib/active_record/autosave_association.rb
@@ -457,10 +457,16 @@ module ActiveRecord
       # If the record is new or it has changed, returns true.
       def record_changed?(reflection, record, key)
         record.new_record? ||
-          (record.has_attribute?(reflection.foreign_key) && record[reflection.foreign_key] != key) ||
+          association_foreign_key_changed?(reflection, record, key) ||
           record.will_save_change_to_attribute?(reflection.foreign_key)
       end
 
+      def association_foreign_key_changed?(reflection, record, key)
+        return false if reflection.through_reflection?
+
+        record.has_attribute?(reflection.foreign_key) && record[reflection.foreign_key] != key
+      end
+
       # Saves the associated record if it's new or <tt>:autosave</tt> is enabled.
       #
       # In addition, it will destroy the association if it was marked for destruction.
diff --git a/activerecord/test/cases/autosave_association_test.rb b/activerecord/test/cases/autosave_association_test.rb
index 54eb885f6a..1a0732c14b 100644
--- a/activerecord/test/cases/autosave_association_test.rb
+++ b/activerecord/test/cases/autosave_association_test.rb
@@ -1,6 +1,7 @@
 # frozen_string_literal: true
 
 require "cases/helper"
+require "models/author"
 require "models/bird"
 require "models/post"
 require "models/comment"
@@ -1319,21 +1320,45 @@ end
 class TestAutosaveAssociationOnAHasOneThroughAssociation < ActiveRecord::TestCase
   self.use_transactional_tests = false unless supports_savepoints?
 
-  def setup
-    super
+  def create_member_with_organization
     organization = Organization.create
-    @member = Member.create
-    MemberDetail.create(organization: organization, member: @member)
+    member = Member.create
+    MemberDetail.create(organization: organization, member: member)
+
+    member
   end
 
   def test_should_not_has_one_through_model
-    class << @member.organization
+    member = create_member_with_organization
+
+    class << member.organization
+      def save(*args)
+        super
+        raise "Oh noes!"
+      end
+    end
+    assert_nothing_raised { member.save }
+  end
+
+  def create_author_with_post_with_comment
+    Author.create! name: "David" # make comment_id not match author_id
+    author = Author.create! name: "Sergiy"
+    post = Post.create! author: author, title: "foo", body: "bar"
+    Comment.create! post: post, body: "cool comment"
+
+    author
+  end
+
+  def test_should_not_reversed_has_one_through_model
+    author = create_author_with_post_with_comment
+
+    class << author.comment_on_first_post
       def save(*args)
         super
         raise "Oh noes!"
       end
     end
-    assert_nothing_raised { @member.save }
+    assert_nothing_raised { author.save }
   end
 end
 
-- 
cgit v1.2.3