From 5b4a516c1c09bb99541b321282f7d4a36c96d693 Mon Sep 17 00:00:00 2001 From: ktmouk Date: Wed, 18 Nov 2020 21:46:49 +0900 Subject: [PATCH] Fixed odd behavior of inverse_of with multiple belongs_to to same class Fix: #35204. This PR added validation to `automatic_inverse_of` that foreign_keys are the same. If class has multiple `belongs_to` to same class, `automatic_inverse_of` can find the wrong `inverse_name`. ```ruby class Room < ActiveRecord::Base belongs_to :user belongs_to :owner, class_name: "User" end class User < ActiveRecord::Base has_one :room has_one :owned_room, class_name: "Room", foreign_key: "owner_id" end user = User.create! owned_room = Room.create!(owner: user) p user.room ``` The current `automatic_inverse_of` validates the `reflection` that found from associations. However, its validation does not validate that foreign keys are the same. so this issue can be fixed by adding a validation of foreign keys. Co-authored-by: alpaca-tc Co-authored-by: cat2koban Co-authored-by: luccafort --- activerecord/CHANGELOG.md | 6 +++++ activerecord/lib/active_record/reflection.rb | 1 + .../associations/inverse_associations_test.rb | 25 +++++++++++++++++++ activerecord/test/models/room.rb | 6 +++++ activerecord/test/models/user.rb | 2 ++ activerecord/test/schema/schema.rb | 5 ++++ 6 files changed, 45 insertions(+) create mode 100644 activerecord/test/models/room.rb diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 8fe0940d6a..e2adc7ba98 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,9 @@ +* Fix odd behavior of inverse_of with multiple belongs_to to same class. + + Fixes #35204. + + *Tomoyuki Kai* + * Build predicate conditions with objects that delegate `#id` and primary key: ```ruby diff --git a/activerecord/lib/active_record/reflection.rb b/activerecord/lib/active_record/reflection.rb index e1b207778d..9083052098 100644 --- a/activerecord/lib/active_record/reflection.rb +++ b/activerecord/lib/active_record/reflection.rb @@ -623,6 +623,7 @@ module ActiveRecord # with the current reflection's klass name. def valid_inverse_reflection?(reflection) reflection && + foreign_key == reflection.foreign_key && klass <= reflection.active_record && can_find_inverse_of_automatically?(reflection) end diff --git a/activerecord/test/cases/associations/inverse_associations_test.rb b/activerecord/test/cases/associations/inverse_associations_test.rb index b8b8858733..e2c76f96f1 100644 --- a/activerecord/test/cases/associations/inverse_associations_test.rb +++ b/activerecord/test/cases/associations/inverse_associations_test.rb @@ -21,6 +21,8 @@ require "models/company" require "models/project" require "models/author" require "models/post" +require "models/user" +require "models/room" class AutomaticInverseFindingTests < ActiveRecord::TestCase fixtures :ratings, :comments, :cars @@ -85,6 +87,29 @@ class AutomaticInverseFindingTests < ActiveRecord::TestCase assert_equal post_reflection, author_child_reflection.inverse_of, "The Author reflection's inverse should be the Post reflection" end + def test_has_one_and_belongs_to_with_non_default_foreign_key_should_not_find_inverse_automatically + user = User.create! + owned_room = Room.create!(owner: user) + + assert_nil user.room + assert_nil owned_room.user + + assert_equal user, owned_room.owner + assert_equal owned_room, user.owned_room + end + + def test_has_one_and_belongs_to_with_custom_association_name_should_not_find_wrong_inverse_automatically + user_reflection = Room.reflect_on_association(:user) + owner_reflection = Room.reflect_on_association(:owner) + room_reflection = User.reflect_on_association(:room) + + assert_predicate user_reflection, :has_inverse? + assert_equal room_reflection, user_reflection.inverse_of + + assert_not_predicate owner_reflection, :has_inverse? + assert_not_equal room_reflection, owner_reflection.inverse_of + 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/models/room.rb b/activerecord/test/models/room.rb new file mode 100644 index 0000000000..cd934418d7 --- /dev/null +++ b/activerecord/test/models/room.rb @@ -0,0 +1,6 @@ +# frozen_string_literal: true + +class Room < ActiveRecord::Base + belongs_to :user + belongs_to :owner, class_name: "User" +end diff --git a/activerecord/test/models/user.rb b/activerecord/test/models/user.rb index 376972aead..a1eb844964 100644 --- a/activerecord/test/models/user.rb +++ b/activerecord/test/models/user.rb @@ -10,6 +10,8 @@ class User < ActiveRecord::Base class_name: "Job", join_table: "jobs_pool" + has_one :room + has_one :owned_room, class_name: "Room", foreign_key: "owner_id" has_one :family_tree, -> { where(token: nil) }, foreign_key: "member_id" has_one :family, through: :family_tree has_many :family_members, through: :family, source: :members diff --git a/activerecord/test/schema/schema.rb b/activerecord/test/schema/schema.rb index 23377911d9..6292a0473c 100644 --- a/activerecord/test/schema/schema.rb +++ b/activerecord/test/schema/schema.rb @@ -873,6 +873,11 @@ ActiveRecord::Schema.define do t.integer :lock_version, default: 0 end + create_table :rooms, force: true do |t| + t.references :user + t.references :owner + end + disable_referential_integrity do create_table :seminars, force: :cascade do |t| t.string :name