Merge pull request rails#41552 from gmcgibbon/has_many_inverse_reflect_self

Fix has_many inversing recursion on models with recursive associations

Author: gmcgibbon

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"]]
     ```

activerecord/lib/active_record/associations.rb

@@ -59,6 +59,18 @@ def corrections
     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
 

activerecord/lib/active_record/reflection.rb

@@ -235,6 +235,9 @@ def check_validity_of_inverse!
           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 @@ def automatic_inverse_of
         # 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)

activerecord/test/cases/associations/inverse_associations_test.rb

@@ -25,6 +25,7 @@
 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 @@ def test_with_hash_many_inversing_does_not_add_duplicate_associated_objects
     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

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

activerecord/test/schema/schema.rb

@@ -145,6 +145,10 @@
     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