Automatically infer inverse_of with scopes

Background
---

I recently noticed we had a number of associations in GitHub that would
benefit from having `inverse_of` set, and so I began adding them. I
ended up adding them to virtually every association with a scope, at
which point I wondered whether Rails might be able to automatically find
these inverses for us.

For GitHub, the changes in this commit end up automatically adding
`inverse_of` to 171 of associations that were missing it.

My understanding is that we do not automatically detect `inverse_of` for
associations with scopes because the scopes could exclude the records we
are trying to inverse from. But I think that should only matter if there
is a scope on the inverse side, not on the association itself.

For example:

Scope on has_many
----

```rb
class Post < ActiveRecord::Base
  has_many :comments, -> { visible }
end

class Comment < ActiveRecord::Base
  belongs_to :post

  scope :visible, -> { where(visible: true) }
  scope :hidden, -> { where(visible: false) }
end

post = Post.create!
comment = post.comments.hidden.create!
assert comment.post
```

This code leaves `post.comments` in sort of a weird state, since it
includes a comment that the association would filter out. But that's
true regardless of the changes in this commit.

Regardless of whether the comments association has an inverse,
`comment.post` will return the post. The difference is that when
`inverse_of` is set we use the existing post we have in memory, rather
than loading it again. If there is a downside to having the `inverse_of`
automatically set here I'm not seeing it.

Scope on belongs_to
----

```rb
class Post < ActiveRecord::Base
  has_many :comments

  scope :visible, -> { where(visible: true) }
  scope :hidden, -> { where(visible: false) }
end

class Comment < ActiveRecord::Base
  belongs_to :post, -> { visible }
end

post = Post.hidden.create!
comment = post.comments.create!
assert_nil comment.post
```

This example is a different story. We don't want to automatically infer
the inverse here because that would change the behavior of
`comment.post`. It should return `nil`, since it's scoped to visible
posts while this one is hidden.

This behavior was not well tested, so this commit adds a test to
ensure we haven't changed it.

Changes
---

This commit changes `can_find_inverse_of_automatically` to allow us to
automatically detect `inverse_of` when there is a scope on the
association, but not when there is a scope on the potential inverse
association. (`can_find_inverse_of_automatically` gets called first with
the association's reflection, then if it returns true we attempt to find
the inverse reflection, and finally we call the method again with the
inverse reflection to ensure we can really use it.)

Since this is a breaking change—specifically in places where code may
have relied on a missing `inverse_of` causing fresh copies of a record
to be loaded—we've placed it behind the `automatic_scope_inversing` flag
(whose name was inspired by `has_many_inversing`). It is set to true for
new applications via framework defaults.

Testing
---

In addition to the inverse association tests, this commit also adds some
cases to a few tests related to preloading. They are basically
duplicates of existing tests, but with lower query counts.

Testing this change with GitHub, the bulk of the failing tests were
related to lower query counts. There were additionally 3 places (2 in
tests and one in application code) where we relied on missing
`inverse_of` causing fresh copies of a record to be loaded.

There's still one Rails test that wouldn't pass if we ran the whole
suite with `automatic_scope_inversing = true`. It's related to
`TaggedPost`, which changes the polymorphic type from the base class
`Post` to the subclass `TaggedPost`.

```rb
class TaggedPost < Post
  has_many :taggings, -> { rewhere(taggable_type: "TaggedPost") }, as: :taggable
end
```

Setting the inverse doesn't work because it ends up changing the type
back to `Post`, something like this:

```rb
post = TaggedPost.new
tagging = post.taggings.new
puts tagging.taggable_type
=> TaggedPost

tagging.taggable = post
puts tagging.taggable_type
=> Post
```

I think this is an acceptable change, given that it's a fairly specific
scenario, and is sort of at odds with the way polymorphic associations
are meant to work (they are meant to contain the base class, not the
subclass). If someone is relying on this specific behavior they can
still either keep `automatic_scope_inversing` set to false, or they can
add `inverse_of: false` to the association.

I haven't found any other cases where having the `inverse_of` would
cause problems like this.

Co-authored-by: Chris Bloom <[email protected]>

Author: composerinteralia

activerecord/CHANGELOG.md

@@ -1,3 +1,32 @@
+*   Allow automatic `inverse_of` detection for associations with scopes.
+
+    Automatic `inverse_of` detection now works for associations with scopes. For
+    example, the `comments` association here now automatically detects
+    `inverse_of: :post`, so we don't need to pass that option:
+
+    ```ruby
+    class Post < ActiveRecord::Base
+      has_many :comments, -> { visible }
+    end
+
+    class Comment < ActiveRecord::Base
+      belongs_to :post
+    end
+    ```
+
+    Note that the automatic detection still won't work if the inverse
+    association has a scope. In this example a scope on the `post` association
+    would still prevent Rails from finding the inverse for the `comments`
+    association.
+
+    This will be the default for new apps in Rails 7. To opt in:
+
+    ```ruby
+    config.active_record.automatic_scope_inversing = true
+    ```
+
+    *Daniel Colson*, *Chris Bloom*
+
 *   Accept optional transaction args to `ActiveRecord::Locking::Pessimistic#with_lock`
 
     `#with_lock` now accepts transaction options like `requires_new:`,

activerecord/lib/active_record/associations.rb

@@ -748,9 +748,10 @@ def association_instance_set(name, association)
       # inverse detection only works on #has_many, #has_one, and
       # #belongs_to associations.
       #
-      # <tt>:foreign_key</tt> and <tt>:through</tt> options on the associations,
-      # or a custom scope, will also prevent the association's inverse
-      # from being found automatically.
+      # <tt>:foreign_key</tt> and <tt>:through</tt> options on the associations
+      # will also prevent the association's inverse from being found automatically,
+      # as will a custom scopes in some cases. See further details in the
+      # {Active Record Associations guide}[https://guides.rubyonrails.org/association_basics.html#bi-directional-associations].
       #
       # The automatic guessing of the inverse association uses a heuristic based
       # on the name of the class, so it may not work for all associations,

activerecord/lib/active_record/reflection.rb

@@ -10,6 +10,7 @@ module Reflection # :nodoc:
     included do
       class_attribute :_reflections, instance_writer: false, default: {}
       class_attribute :aggregate_reflections, instance_writer: false, default: {}
+      class_attribute :automatic_scope_inversing, instance_writer: false, default: false
     end
 
     class << self
@@ -633,7 +634,7 @@ def valid_inverse_reflection?(reflection)
           reflection &&
             foreign_key == reflection.foreign_key &&
             klass <= reflection.active_record &&
-            can_find_inverse_of_automatically?(reflection)
+            can_find_inverse_of_automatically?(reflection, true)
         end
 
         # Checks to see if the reflection doesn't have any options that prevent
@@ -642,14 +643,25 @@ def valid_inverse_reflection?(reflection)
         # have <tt>has_many</tt>, <tt>has_one</tt>, <tt>belongs_to</tt> associations.
         # Third, we must not have options such as <tt>:foreign_key</tt>
         # which prevent us from correctly guessing the inverse association.
-        #
-        # Anything with a scope can additionally ruin our attempt at finding an
-        # inverse, so we exclude reflections with scopes.
-        def can_find_inverse_of_automatically?(reflection)
+        def can_find_inverse_of_automatically?(reflection, inverse_reflection = false)
           reflection.options[:inverse_of] != false &&
             !reflection.options[:through] &&
             !reflection.options[:foreign_key] &&
+            scope_allows_automatic_inverse_of?(reflection, inverse_reflection)
+        end
+
+        # Scopes on the potential inverse reflection prevent automatic
+        # <tt>inverse_of</tt>, since the scope could exclude the owner record
+        # we would inverse from. Scopes on the reflection itself allow for
+        # automatic <tt>inverse_of</tt> as long as
+        # <tt>config.active_record.automatic_scope_inversing<tt> is set to
+        # +true+ (the default for new applications).
+        def scope_allows_automatic_inverse_of?(reflection, inverse_reflection)
+          if inverse_reflection
             !reflection.scope
+          else
+            !reflection.scope || reflection.klass.automatic_scope_inversing
+          end
         end
 
         def derive_class_name
@@ -736,7 +748,7 @@ def join_foreign_type
       end
 
       private
-        def can_find_inverse_of_automatically?(_)
+        def can_find_inverse_of_automatically?(*)
           !polymorphic? && super
         end
     end

activerecord/test/cases/associations/inverse_associations_test.rb

@@ -22,9 +22,12 @@
 require "models/author"
 require "models/user"
 require "models/room"
+require "models/contract"
+require "models/subscription"
+require "models/book"
 
 class AutomaticInverseFindingTests < ActiveRecord::TestCase
-  fixtures :ratings, :comments, :cars
+  fixtures :ratings, :comments, :cars, :books
 
   def test_has_one_and_belongs_to_should_find_inverse_automatically_on_multiple_word_name
     monkey_reflection = MixedCaseMonkey.reflect_on_association(:human)
@@ -109,6 +112,51 @@ def test_has_one_and_belongs_to_with_custom_association_name_should_not_find_wro
     assert_not_equal room_reflection, owner_reflection.inverse_of
   end
 
+  def test_has_many_and_belongs_to_with_a_scope_and_automatic_scope_inversing_should_find_inverse_automatically
+    contacts_reflection = Company.reflect_on_association(:special_contracts)
+    company_reflection = SpecialContract.reflect_on_association(:company)
+
+    assert contacts_reflection.scope
+    assert_not company_reflection.scope
+
+    with_automatic_scope_inversing(contacts_reflection, company_reflection) do
+      assert_predicate contacts_reflection, :has_inverse?
+      assert_equal company_reflection, contacts_reflection.inverse_of
+      assert_not_equal contacts_reflection, company_reflection.inverse_of
+    end
+  end
+
+  def test_has_one_and_belongs_to_with_a_scope_and_automatic_scope_inversing_should_find_inverse_automatically
+    post_reflection = Author.reflect_on_association(:recent_post)
+    author_reflection = Post.reflect_on_association(:author)
+
+    assert post_reflection.scope
+    assert_not author_reflection.scope
+
+    with_automatic_scope_inversing(post_reflection, author_reflection) do
+      assert_predicate post_reflection, :has_inverse?
+      assert_equal author_reflection, post_reflection.inverse_of
+      assert_not_equal post_reflection, author_reflection.inverse_of
+    end
+  end
+
+  def test_has_many_with_scoped_belongs_to_does_not_find_inverse_automatically
+    book = books(:tlg)
+    book.update_attribute(:author_visibility, :invisible)
+
+    assert_nil book.subscriptions.new.book
+
+    subscription_reflection = Book.reflect_on_association(:subscriptions)
+    book_reflection = Subscription.reflect_on_association(:book)
+
+    assert_not subscription_reflection.scope
+    assert book_reflection.scope
+
+    with_automatic_scope_inversing(book_reflection, subscription_reflection) do
+      assert_nil book.subscriptions.new.book
+    end
+  end
+
   def test_has_one_and_belongs_to_automatic_inverse_shares_objects
     car = Car.first
     bulb = Bulb.create!(car: car)

activerecord/test/cases/associations/nested_through_associations_test.rb

@@ -68,6 +68,17 @@ def test_has_many_through_has_many_with_has_many_through_source_reflection_prelo
     assert_no_queries do
       assert_equal [general, general], author.tags
     end
+
+    # Preloading with automatic scope inversing reduces the number of queries
+    tag_reflection = Tagging.reflect_on_association(:tag)
+    taggings_reflection = Tag.reflect_on_association(:taggings)
+
+    assert tag_reflection.scope
+    assert_not taggings_reflection.scope
+
+    with_automatic_scope_inversing(tag_reflection, taggings_reflection) do
+      assert_queries(4) { Author.includes(:tags).first }
+    end
   end
 
   def test_has_many_through_has_many_with_has_many_through_source_reflection_preload_via_joins
@@ -309,6 +320,17 @@ def test_has_many_through_has_many_through_with_belongs_to_source_reflection_pre
     assert_no_queries do
       assert_equal [general, general], author.tagging_tags
     end
+
+    # Preloading with automatic scope inversing reduces the number of queries
+    tag_reflection = Tagging.reflect_on_association(:tag)
+    taggings_reflection = Tag.reflect_on_association(:taggings)
+
+    assert tag_reflection.scope
+    assert_not taggings_reflection.scope
+
+    with_automatic_scope_inversing(tag_reflection, taggings_reflection) do
+      assert_queries(4) { Author.includes(:tagging_tags).first }
+    end
   end
 
   def test_has_many_through_has_many_through_with_belongs_to_source_reflection_preload_via_joins

activerecord/test/cases/associations_test.rb

@@ -702,15 +702,33 @@ def test_preload_can_group_multi_level_ping_pong_through
 
     AuthorFavorite.create!(author: mary, favorite_author: bob)
 
+    associations = { similar_posts: :comments, favorite_authors: { similar_posts: :comments } }
+
     assert_queries(9) do
-      preloader = ActiveRecord::Associations::Preloader.new(records: [mary], associations: { similar_posts: :comments, favorite_authors: { similar_posts: :comments } })
+      preloader = ActiveRecord::Associations::Preloader.new(records: [mary], associations: associations)
       preloader.call
     end
 
     assert_no_queries do
       mary.similar_posts.map(&:comments).each(&:to_a)
       mary.favorite_authors.flat_map(&:similar_posts).map(&:comments).each(&:to_a)
     end
+
+    # Preloading with automatic scope inversing reduces the number of queries
+    tag_reflection = Tagging.reflect_on_association(:tag)
+    taggings_reflection = Tag.reflect_on_association(:taggings)
+
+    assert tag_reflection.scope
+    assert_not taggings_reflection.scope
+
+    with_automatic_scope_inversing(tag_reflection, taggings_reflection) do
+      mary.reload
+
+      assert_queries(8) do
+        preloader = ActiveRecord::Associations::Preloader.new(records: [mary], associations: associations)
+        preloader.call
+      end
+    end
   end
 
   def test_preload_does_not_group_same_class_different_scope

activerecord/test/cases/test_case.rb

@@ -91,6 +91,24 @@ def with_has_many_inversing(model = ActiveRecord::Base)
       end
     end
 
+    def with_automatic_scope_inversing(*reflections)
+      old = reflections.map { |reflection| reflection.klass.automatic_scope_inversing }
+
+      reflections.each do |reflection|
+        reflection.klass.automatic_scope_inversing = true
+        reflection.remove_instance_variable(:@inverse_name) if reflection.instance_variable_defined?(:@inverse_name)
+        reflection.remove_instance_variable(:@inverse_of) if reflection.instance_variable_defined?(:@inverse_of)
+      end
+
+      yield
+    ensure
+      reflections.each_with_index do |reflection, i|
+        reflection.klass.automatic_scope_inversing = old[i]
+        reflection.remove_instance_variable(:@inverse_name) if reflection.instance_variable_defined?(:@inverse_name)
+        reflection.remove_instance_variable(:@inverse_of) if reflection.instance_variable_defined?(:@inverse_of)
+      end
+    end
+
     def reset_callbacks(klass, kind)
       old_callbacks = {}
       old_callbacks[klass] = klass.send("_#{kind}_callbacks").dup

activerecord/test/models/subscription.rb

@@ -2,7 +2,7 @@
 
 class Subscription < ActiveRecord::Base
   belongs_to :subscriber, counter_cache: :books_count
-  belongs_to :book
+  belongs_to :book, -> { author_visibility_visible }
 
   validates_presence_of :subscriber_id, :book_id
 end

guides/source/association_basics.md

@@ -773,10 +773,13 @@ irb> a.first_name == b.author.first_name
 => true
 ```
 
-Active Record supports automatic identification for most associations with standard names. However, Active Record will not automatically identify bi-directional associations that contain a scope or any of the following options:
-
-* `:through`
-* `:foreign_key`
+Active Record supports automatic identification for most associations with
+standard names. However, Active Record will not automatically identify
+bi-directional associations that contain the `:through` or `:foreign_key`
+options. Custom scopes on the opposite association also prevent automatic
+identification, as do custom scopes on the association itself unless
+`config.active_record.automatic_scope_inversing` is set to true (the default for
+new applications).
 
 For example, consider the following model declarations:
 

guides/source/configuring.md

@@ -711,6 +711,10 @@ support recycling cache key.
 Enables setting the inverse record when traversing `belongs_to` to `has_many`
 associations.
 
+#### `config.active_record.automatic_scope_inversing`
+
+Enables automatically inferring the `inverse_of` for associations with a scope.
+
 #### `config.active_record.legacy_connection_handling`
 
 Allows to enable new connection handling API. For applications using multiple
@@ -1701,6 +1705,7 @@ Accepts a string for the HTML tag used to wrap attachments. Defaults to `"action
 - `config.action_controller.silence_disabled_session_errors`: `false`
 - `config.action_mailer.smtp_timeout`: `5`
 - `config.active_storage.video_preview_arguments`: `"-vf 'select=eq(n\\,0)+eq(key\\,1)+gt(scene\\,0.015),loop=loop=-1:size=2,trim=start_frame=1' -frames:v 1 -f image2"`
+- `config.active_record.automatic_scope_inversing`: `true`
 - `config.active_record.verify_foreign_keys_for_fixtures`: `true`
 - `config.active_record.partial_inserts`: `false`
 - `config.active_storage.variant_processor`: `:vips`