From a77f81cfeb642ce16c01a39ff12088e6a6106456 Mon Sep 17 00:00:00 2001 From: Gannon McGibbon Date: Thu, 25 Feb 2021 20:36:12 -0500 Subject: [PATCH] Fix has_many inversing recursion on models with recursive associations --- activerecord/CHANGELOG.md | 8 ++++-- .../lib/active_record/associations.rb | 12 +++++++++ activerecord/lib/active_record/reflection.rb | 4 +++ .../associations/inverse_associations_test.rb | 25 +++++++++++++++++++ activerecord/test/models/branch.rb | 11 ++++++++ activerecord/test/schema/schema.rb | 4 +++ 6 files changed, 62 insertions(+), 2 deletions(-) create mode 100644 activerecord/test/models/branch.rb diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 623cb17891..c50e94e338 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,7 @@ +* Fix `has_many` inversing recursion on models with recursive associations. + + *Gannon McGibbon* + * Add nested_attributes_for support for `delegated_type` ```ruby @@ -33,7 +37,7 @@ Prior to this change, deletes with GROUP_BY and HAVING were returning an error. - After this change, GROUP_BY and HAVING are valid clauses in DELETE queries, generating the following query: + After this change, GROUP_BY and HAVING are valid clauses in DELETE queries, generating the following query: ```sql DELETE FROM "posts" WHERE "posts"."id" IN ( @@ -57,7 +61,7 @@ ```sql UPDATE "posts" SET "flagged" = ? WHERE "posts"."id" IN ( - SELECT "posts"."id" FROM "posts" INNER JOIN "comments" ON "comments"."post_id" = "posts"."id" + SELECT "posts"."id" FROM "posts" INNER JOIN "comments" ON "comments"."post_id" = "posts"."id" GROUP BY posts.id HAVING (count(comments.id) >= 2) ) [["flagged", "t"]] ``` diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index 35a8aa2939..1c60aef2d9 100644 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -59,6 +59,18 @@ module ActiveRecord end end + class InverseOfAssociationRecursiveError < ActiveRecordError # :nodoc: + attr_reader :reflection + def initialize(reflection = nil) + if reflection + @reflection = reflection + super("Inverse association #{reflection.name} (#{reflection.options[:inverse_of].inspect} in #{reflection.class_name}) is recursive.") + else + super("Inverse association is recursive.") + end + end + end + class HasManyThroughAssociationNotFoundError < ActiveRecordError # :nodoc: attr_reader :owner_class, :reflection diff --git a/activerecord/lib/active_record/reflection.rb b/activerecord/lib/active_record/reflection.rb index 9410de8cb1..4c74473ea1 100644 --- a/activerecord/lib/active_record/reflection.rb +++ b/activerecord/lib/active_record/reflection.rb @@ -235,6 +235,9 @@ module ActiveRecord if has_inverse? && inverse_of.nil? raise InverseOfAssociationNotFoundError.new(self) end + if has_inverse? && inverse_of == self + raise InverseOfAssociationRecursiveError.new(self) + end end end @@ -632,6 +635,7 @@ module ActiveRecord # with the current reflection's klass name. def valid_inverse_reflection?(reflection) reflection && + reflection != self && foreign_key == reflection.foreign_key && klass <= reflection.active_record && can_find_inverse_of_automatically?(reflection, true) diff --git a/activerecord/test/cases/associations/inverse_associations_test.rb b/activerecord/test/cases/associations/inverse_associations_test.rb index 55683260c4..54da5a24ea 100644 --- a/activerecord/test/cases/associations/inverse_associations_test.rb +++ b/activerecord/test/cases/associations/inverse_associations_test.rb @@ -25,6 +25,7 @@ require "models/room" require "models/contract" require "models/subscription" require "models/book" +require "models/branch" class AutomaticInverseFindingTests < ActiveRecord::TestCase fixtures :ratings, :comments, :cars, :books @@ -787,6 +788,30 @@ class InverseBelongsToTests < ActiveRecord::TestCase end end + def test_recursive_model_has_many_inversing + with_has_many_inversing do + main = Branch.create! + feature = main.branches.create! + topic = feature.branches.build + + assert_equal(main, topic.branch.branch) + end + end + + def test_recursive_inverse_on_recursive_model_has_many_inversing + with_has_many_inversing do + main = BrokenBranch.create! + feature = main.branches.create! + topic = feature.branches.build + + error = assert_raises(ActiveRecord::InverseOfAssociationRecursiveError) do + topic.branch.branch + end + + assert_equal("Inverse association branch (:branch in BrokenBranch) is recursive.", error.message) + end + end + def test_unscope_does_not_set_inverse_when_incorrect interest = interests(:trainspotting) human = interest.human diff --git a/activerecord/test/models/branch.rb b/activerecord/test/models/branch.rb new file mode 100644 index 0000000000..231b007709 --- /dev/null +++ b/activerecord/test/models/branch.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +class Branch < ActiveRecord::Base + has_many :branches + belongs_to :branch, optional: true +end + +class BrokenBranch < Branch + has_many :branches, class_name: "BrokenBranch", foreign_key: :branch_id + belongs_to :branch, optional: true, inverse_of: :branch, class_name: "BrokenBranch" +end diff --git a/activerecord/test/schema/schema.rb b/activerecord/test/schema/schema.rb index 6070b82d43..056d59dcac 100644 --- a/activerecord/test/schema/schema.rb +++ b/activerecord/test/schema/schema.rb @@ -145,6 +145,10 @@ ActiveRecord::Schema.define do t.boolean :has_fun, null: false, default: false end + create_table :branches, force: true do |t| + t.references :branch + end + create_table :bulbs, primary_key: "ID", force: true do |t| t.integer :car_id t.string :name