From 2dd81877a48a8b087c07eee74119c21d9e1ff892 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafael=20Mendon=C3=A7a=20Fran=C3=A7a?= Date: Thu, 7 Jan 2021 22:15:24 +0000 Subject: [PATCH] Fix has_many_inversing with touch: true on a belongs_to association with inverse MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `has_many_inversing` adds records to a has_many association. It does so with destroyed records, too. So if a child was destroyed with a `touch: true` association on the parent *and* the parent was not loaded, it tried to load the parent to touch it. While loading the parent it added the child record to the parent's has_many association. The logic doing this always set the child's parent id – even if it was correct/the same already. But since the child is destroyed, it resulted in a `FrozenError`. This commit prevents doing the unnecessary setting of the identical id and therefore fixes this error. Fixes #40943 by not doing an unneeded attribute set. Closes #40969. [Markus Doits + Rafael Mendonça França] --- .../associations/belongs_to_association.rb | 10 +++++-- .../belongs_to_polymorphic_association.rb | 9 ++++-- .../belongs_to_associations_test.rb | 28 +++++++++++++++++++ activerecord/test/models/book.rb | 2 +- activerecord/test/models/citation.rb | 1 + activerecord/test/models/toy.rb | 2 ++ 6 files changed, 46 insertions(+), 6 deletions(-) diff --git a/activerecord/lib/active_record/associations/belongs_to_association.rb b/activerecord/lib/active_record/associations/belongs_to_association.rb index 8aa3f7c20e..ec8deca806 100644 --- a/activerecord/lib/active_record/associations/belongs_to_association.rb +++ b/activerecord/lib/active_record/associations/belongs_to_association.rb @@ -80,7 +80,7 @@ module ActiveRecord @updated = true end - replace_keys(record) + replace_keys(record, force: true) self.target = record end @@ -108,8 +108,12 @@ module ActiveRecord reflection.counter_cache_column && owner.persisted? end - def replace_keys(record) - owner[reflection.foreign_key] = record ? record._read_attribute(primary_key(record.class)) : nil + def replace_keys(record, force: false) + target_key = record ? record._read_attribute(primary_key(record.class)) : nil + + if force || owner[reflection.foreign_key] != target_key + owner[reflection.foreign_key] = target_key + end end def primary_key(klass) diff --git a/activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb b/activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb index 6f17b755db..a0d5522fa0 100644 --- a/activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb +++ b/activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb @@ -14,9 +14,14 @@ module ActiveRecord end private - def replace_keys(record) + def replace_keys(record, force: false) super - owner[reflection.foreign_type] = record ? record.class.polymorphic_name : nil + + target_type = record ? record.class.polymorphic_name : nil + + if force || owner[reflection.foreign_type] != target_type + owner[reflection.foreign_type] = target_type + end end def inverse_reflection_for(record) diff --git a/activerecord/test/cases/associations/belongs_to_associations_test.rb b/activerecord/test/cases/associations/belongs_to_associations_test.rb index 62f32e61be..c252bb1b69 100644 --- a/activerecord/test/cases/associations/belongs_to_associations_test.rb +++ b/activerecord/test/cases/associations/belongs_to_associations_test.rb @@ -25,6 +25,8 @@ require "models/admin/user" require "models/ship" require "models/treasure" require "models/parrot" +require "models/book" +require "models/citation" class BelongsToAssociationsTest < ActiveRecord::TestCase fixtures :accounts, :companies, :developers, :projects, :topics, @@ -1177,6 +1179,17 @@ class BelongsToAssociationsTest < ActiveRecord::TestCase assert_equal companies(:another_firm), client.firm_with_condition end + def test_destroying_child_with_unloaded_parent_and_foreign_key_and_touch_is_possible_with_has_many_inversing + with_has_many_inversing do + book = Book.create! + citation = book.citations.create! + + assert_difference "Citation.count", -1 do + Citation.find(citation.id).destroy + end + end + end + def test_polymorphic_reassignment_of_associated_id_updates_the_object sponsor = sponsors(:moustache_club_sponsor_for_groucho) @@ -1359,6 +1372,21 @@ class BelongsToAssociationsTest < ActiveRecord::TestCase assert_equal toy, sponsor.reload.sponsorable end + class SponsorWithTouchInverse < Sponsor + belongs_to :sponsorable, polymorphic: true, inverse_of: :sponsors, touch: true + end + + def test_destroying_polymorphic_child_with_unloaded_parent_and_touch_is_possible_with_has_many_inversing + with_has_many_inversing do + toy = Toy.create! + sponsor = toy.sponsors.create! + + assert_difference "Sponsor.count", -1 do + SponsorWithTouchInverse.find(sponsor.id).destroy + end + end + end + def test_polymorphic_with_false assert_nothing_raised do Class.new(ActiveRecord::Base) do diff --git a/activerecord/test/models/book.rb b/activerecord/test/models/book.rb index 78ea179f01..a83e785b2b 100644 --- a/activerecord/test/models/book.rb +++ b/activerecord/test/models/book.rb @@ -3,7 +3,7 @@ class Book < ActiveRecord::Base belongs_to :author - has_many :citations, foreign_key: "book1_id" + has_many :citations, foreign_key: "book1_id", inverse_of: :book has_many :references, -> { distinct }, through: :citations, source: :reference_of has_many :subscriptions diff --git a/activerecord/test/models/citation.rb b/activerecord/test/models/citation.rb index cee3d18173..6361e131eb 100644 --- a/activerecord/test/models/citation.rb +++ b/activerecord/test/models/citation.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true class Citation < ActiveRecord::Base + belongs_to :book, foreign_key: :book1_id, inverse_of: :citations, touch: true belongs_to :reference_of, class_name: "Book", foreign_key: :book2_id has_many :citations end diff --git a/activerecord/test/models/toy.rb b/activerecord/test/models/toy.rb index 4a5697eeb1..d405078f96 100644 --- a/activerecord/test/models/toy.rb +++ b/activerecord/test/models/toy.rb @@ -4,5 +4,7 @@ class Toy < ActiveRecord::Base self.primary_key = :toy_id belongs_to :pet + has_many :sponsors, as: :sponsorable, inverse_of: :sponsorable + scope :with_pet, -> { joins(:pet) } end