Merge pull request #43358 from composerinteralia/automatic-inverse-of…

…-with-scopes

Automatically infer inverse_of with scopes

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`

railties/lib/rails/application/configuration.rb

@@ -234,6 +234,7 @@ def load_defaults(target_version)
           if respond_to?(:active_record)
             active_record.verify_foreign_keys_for_fixtures = true
             active_record.partial_inserts = false
+            active_record.automatic_scope_inversing = true
           end
 
           if respond_to?(:action_controller)

railties/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_7_0.rb.tt

@@ -54,6 +54,9 @@
 # Rails.application.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"
 
+# Automatically infer `inverse_of` for associations with a scope.
+# Rails.application.config.active_record.automatic_scope_inversing = true
+
 # Raise when running tests if fixtures contained foreign key violations
 # Rails.application.config.active_record.verify_foreign_keys_for_fixtures = true
 

railties/test/application/configuration_test.rb

@@ -2266,6 +2266,32 @@ def index
       assert_equal true, ActiveRecord::Base.has_many_inversing
     end
 
+    test "ActiveRecord::Base.automatic_scope_inversing is true by default for new apps" do
+      app "development"
+
+      assert_equal true, ActiveRecord::Base.automatic_scope_inversing
+    end
+
+    test "ActiveRecord::Base.automatic_scope_inversing is false by default for upgraded apps" do
+      remove_from_config '.*config\.load_defaults.*\n'
+
+      app "development"
+
+      assert_equal false, ActiveRecord::Base.automatic_scope_inversing
+    end
+
+    test "ActiveRecord::Base.automatic_scope_inversing can be configured via config.active_record.automatic_scope_inversing" do
+      remove_from_config '.*config\.load_defaults.*\n'
+
+      app_file "config/initializers/new_framework_defaults_7_0.rb", <<-RUBY
+        Rails.application.config.active_record.automatic_scope_inversing = true
+      RUBY
+
+      app "development"
+
+      assert_equal true, ActiveRecord::Base.automatic_scope_inversing
+    end
+
     test "ActiveRecord.verify_foreign_keys_for_fixtures is true by default for new apps" do
       app "development"