diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 64085770d3..846eb50452 100644 --- a/activerecord/CHANGELOG.md +++ b/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:`, diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index eda4fbace2..35a8aa2939 100644 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -748,9 +748,10 @@ module ActiveRecord # inverse detection only works on #has_many, #has_one, and # #belongs_to associations. # - # :foreign_key and :through options on the associations, - # or a custom scope, will also prevent the association's inverse - # from being found automatically. + # :foreign_key and :through 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, diff --git a/activerecord/lib/active_record/reflection.rb b/activerecord/lib/active_record/reflection.rb index b1d5bbcf8a..9410de8cb1 100644 --- a/activerecord/lib/active_record/reflection.rb +++ b/activerecord/lib/active_record/reflection.rb @@ -10,6 +10,7 @@ module ActiveRecord 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 @@ module ActiveRecord 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 @@ module ActiveRecord # have has_many, has_one, belongs_to associations. # Third, we must not have options such as :foreign_key # 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 + # inverse_of, since the scope could exclude the owner record + # we would inverse from. Scopes on the reflection itself allow for + # automatic inverse_of as long as + # config.active_record.automatic_scope_inversing 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 @@ module ActiveRecord end private - def can_find_inverse_of_automatically?(_) + def can_find_inverse_of_automatically?(*) !polymorphic? && super end end diff --git a/activerecord/test/cases/associations/inverse_associations_test.rb b/activerecord/test/cases/associations/inverse_associations_test.rb index 8c855f66ac..0bb1470981 100644 --- a/activerecord/test/cases/associations/inverse_associations_test.rb +++ b/activerecord/test/cases/associations/inverse_associations_test.rb @@ -22,9 +22,12 @@ require "models/project" 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 @@ class AutomaticInverseFindingTests < ActiveRecord::TestCase 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) diff --git a/activerecord/test/cases/associations/nested_through_associations_test.rb b/activerecord/test/cases/associations/nested_through_associations_test.rb index c763a37c53..aca3f3d83f 100644 --- a/activerecord/test/cases/associations/nested_through_associations_test.rb +++ b/activerecord/test/cases/associations/nested_through_associations_test.rb @@ -68,6 +68,17 @@ class NestedThroughAssociationsTest < ActiveRecord::TestCase 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 @@ class NestedThroughAssociationsTest < ActiveRecord::TestCase 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 diff --git a/activerecord/test/cases/associations_test.rb b/activerecord/test/cases/associations_test.rb index 06050fd129..ba236fe979 100644 --- a/activerecord/test/cases/associations_test.rb +++ b/activerecord/test/cases/associations_test.rb @@ -702,8 +702,10 @@ class PreloaderTest < ActiveRecord::TestCase 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 @@ -711,6 +713,22 @@ class PreloaderTest < ActiveRecord::TestCase 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 diff --git a/activerecord/test/cases/test_case.rb b/activerecord/test/cases/test_case.rb index 2c110e8046..33923c4b66 100644 --- a/activerecord/test/cases/test_case.rb +++ b/activerecord/test/cases/test_case.rb @@ -91,6 +91,24 @@ module ActiveRecord 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 diff --git a/activerecord/test/models/subscription.rb b/activerecord/test/models/subscription.rb index f87315fcd1..9b657b43d9 100644 --- a/activerecord/test/models/subscription.rb +++ b/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 diff --git a/guides/source/association_basics.md b/guides/source/association_basics.md index 80ed8f0ae8..da5df54cf5 100644 --- a/guides/source/association_basics.md +++ b/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: diff --git a/guides/source/configuring.md b/guides/source/configuring.md index 052e8ae4f8..bd5e814ee7 100644 --- a/guides/source/configuring.md +++ b/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` diff --git a/railties/lib/rails/application/configuration.rb b/railties/lib/rails/application/configuration.rb index f83df20a8b..bccfa43701 100644 --- a/railties/lib/rails/application/configuration.rb +++ b/railties/lib/rails/application/configuration.rb @@ -234,6 +234,7 @@ module Rails 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) diff --git a/railties/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_7_0.rb.tt b/railties/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_7_0.rb.tt index 059a26930a..65c452f37a 100644 --- a/railties/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_7_0.rb.tt +++ b/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 diff --git a/railties/test/application/configuration_test.rb b/railties/test/application/configuration_test.rb index 2f1b0e3eca..eba2ad69d3 100644 --- a/railties/test/application/configuration_test.rb +++ b/railties/test/application/configuration_test.rb @@ -2266,6 +2266,32 @@ module ApplicationTests 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"