Merge pull request rails#54280 from Shopify/ac-through-assoc-reverts

Revert has-many-through association changes + add regression test

Author: eileencodes

activerecord/lib/active_record/associations/association.rb

@@ -188,16 +188,6 @@ def extensions
       # not reraised. The proxy is \reset and +nil+ is the return value.
       def load_target
         @target = find_target(async: false) if (@stale_state && stale_target?) || find_target?
-        if !@target && set_through_target_for_new_record?
-          reflections = reflection.chain
-          reflections.pop
-          reflections.reverse!
-
-          @target = reflections.reduce(through_association.target) do |middle_target, through_reflection|
-            break unless middle_target
-            middle_target.association(through_reflection.source_reflection_name).load_target
-          end
-        end
 
         loaded! unless loaded?
         target
@@ -331,10 +321,6 @@ def find_target?
           !loaded? && (!owner.new_record? || foreign_key_present?) && klass
         end
 
-        def set_through_target_for_new_record?
-          owner.new_record? && reflection.through_reflection? && through_association.target
-        end
-
         # Returns true if there is a foreign key present on the owner which
         # references the target. This is used to determine whether we can load
         # the target if the owner is currently a new record (and therefore

activerecord/lib/active_record/associations/collection_association.rb

@@ -272,18 +272,6 @@ def include?(record)
       def load_target
         if find_target?
           @target = merge_target_lists(find_target, target)
-        elsif target.empty? && set_through_target_for_new_record?
-          reflections = reflection.chain.reverse!
-
-          @target = reflections.each_cons(2).reduce(through_association.target) do |middle_target, (middle_reflection, through_reflection)|
-            if middle_target.nil? || (middle_reflection.collection? && middle_target.empty?)
-              break []
-            elsif middle_reflection.collection?
-              middle_target.flat_map { |record| record.association(through_reflection.source_reflection_name).load_target }.compact
-            else
-              middle_target.association(through_reflection.source_reflection_name).load_target
-            end
-          end
         end
 
         loaded!

activerecord/lib/active_record/autosave_association.rb

@@ -297,15 +297,7 @@ def init_internals
       # unless the parent is/was a new record itself.
       def associated_records_to_validate_or_save(association, new_record, autosave)
         if new_record || custom_validation_context?
-          target = association && association.target
-          if target && association.reflection.through_reflection
-            # We expect new through records to be autosaved by their direct parent.
-            # This prevents already persisted through records from being validated or saved
-            # more than once.
-            target.find_all(&:changed_for_autosave?)
-          else
-            target
-          end
+          association && association.target
         elsif autosave
           association.target.find_all(&:changed_for_autosave?)
         else

activerecord/lib/active_record/reflection.rb

@@ -655,10 +655,6 @@ def source_reflection
         self
       end
 
-      def source_reflection_name
-        source_reflection.name
-      end
-
       # A chain of reflections from this one back to the owner. For more see the explanation in
       # ThroughReflection.
       def collect_join_chain
@@ -1235,7 +1231,7 @@ def derive_class_name
 
     class PolymorphicReflection < AbstractReflection # :nodoc:
       delegate :klass, :scope, :plural_name, :type, :join_primary_key, :join_foreign_key,
-               :name, :scope_for, :collection?, to: :@reflection
+               :name, :scope_for, to: :@reflection
 
       def initialize(reflection, previous_reflection)
         super()

activerecord/test/cases/associations/has_many_through_associations_test.rb

@@ -29,6 +29,9 @@
 require "models/member"
 require "models/membership"
 require "models/club"
+require "models/program"
+require "models/program_offering"
+require "models/enrollment"
 require "models/organization"
 require "models/user"
 require "models/family"
@@ -41,7 +44,6 @@
 require "models/zine"
 require "models/interest"
 require "models/human"
-require "models/account"
 
 class HasManyThroughAssociationsTest < ActiveRecord::TestCase
   fixtures :posts, :readers, :people, :comments, :authors, :categories, :taggings, :tags,
@@ -57,122 +59,6 @@ def setup
     Reader.create person_id: 0, post_id: 0
   end
 
-  def test_setting_has_many_through_one_association_on_new_record_sets_through_records
-    account_1, account_2 = Account.create!(credit_limit: 100), Account.create!(credit_limit: 100)
-    firm = Firm.new(accounts: [account_1, account_2])
-    client = Client.new
-    client.firm = firm
-
-    assert_predicate account_1, :persisted?
-    assert_predicate account_2, :persisted?
-    assert_predicate client, :new_record?
-    assert_predicate client.firm, :new_record?
-    assert_no_queries { assert_equal [account_1, account_2].sort, client.accounts.sort }
-  end
-
-  def test_setting_has_many_through_many_association_on_new_record_sets_through_records
-    subscriber_1, subscriber_2 = Subscriber.create!(nick: "nick 1"), Subscriber.create!(nick: "nick 2")
-    subscription_1 = Subscription.new(subscriber: subscriber_1)
-    subscription_2 = Subscription.new(subscriber: subscriber_2)
-    book = Book.new
-    book.subscriptions = [subscription_1, subscription_2]
-
-    assert_predicate subscriber_1, :persisted?
-    assert_predicate subscriber_2, :persisted?
-    assert_predicate book, :new_record?
-    book.subscriptions.each { |subscription| assert_predicate subscription, :new_record? }
-    assert_no_queries { assert_equal [subscriber_1, subscriber_2].sort, book.subscribers.sort }
-  end
-
-  def test_setting_has_many_through_many_association_with_missing_targets_on_new_record_sets_empty_through_records
-    subscription_1 = Subscription.new
-    subscription_2 = Subscription.new
-    book = Book.new
-    book.subscriptions = [subscription_1, subscription_2]
-
-    assert_predicate book, :new_record?
-    book.subscriptions.each { |subscription| assert_predicate subscription, :new_record? }
-    assert_no_queries { assert_equal [], book.subscribers }
-  end
-
-  def test_setting_has_many_through_many_association_with_partial_missing_targets_on_new_record_sets_partial_through_records
-    subscriber_1 = Subscriber.create!(nick: "nick 1")
-    subscription_1 = Subscription.new(subscriber: subscriber_1)
-    subscription_2 = Subscription.new
-    book = Book.new
-    book.subscriptions = [subscription_1, subscription_2]
-
-    assert_predicate subscriber_1, :persisted?
-    assert_predicate book, :new_record?
-    book.subscriptions.each { |subscription| assert_predicate subscription, :new_record? }
-    assert_no_queries { assert_equal [subscriber_1], book.subscribers }
-  end
-
-  def test_setting_polymorphic_has_many_through_many_association_on_new_record_sets_through_records
-    human_1, human_2 = Human.create!, Human.create!
-    interest_1 = Interest.new(polymorphic_human: human_1)
-    interest_2 = Interest.new(polymorphic_human: human_2)
-    zine = Zine.new
-    zine.interests = [interest_1, interest_2]
-
-    assert_predicate human_1, :persisted?
-    assert_predicate human_2, :persisted?
-    assert_predicate zine, :new_record?
-    zine.interests.each { |interest| assert_predicate interest, :new_record? }
-    assert_no_queries { assert_equal [human_1, human_2].sort, zine.polymorphic_humans.sort }
-  end
-
-  def test_setting_nested_has_many_through_one_association_on_new_record_sets_nested_through_records
-    post_tagging_1, post_tagging_2 = Tagging.create!, Tagging.create!
-    post = Post.create!(title: "Tagged", body: "Post", taggings: [post_tagging_1, post_tagging_2])
-    author = Author.new(name: "Josh")
-    author.posts = [post]
-    categorization = Categorization.new
-    categorization.author = author
-
-    assert_predicate post_tagging_1, :persisted?
-    assert_predicate post_tagging_2, :persisted?
-    assert_predicate post, :persisted?
-    assert_predicate categorization, :new_record?
-    assert_predicate categorization.author, :new_record?
-    assert_no_queries { assert_equal [post_tagging_1, post_tagging_2].sort, categorization.post_taggings.sort }
-  end
-
-  def test_setting_nested_has_many_through_one_association_on_new_record_sets_targetless_nested_through_records
-    post = Post.create!(title: "Tagged", body: "Post")
-    post_tagging_1, post_tagging_2 = Tagging.create!(taggable: post), Tagging.create!(taggable: post)
-    author = Author.new(name: "Josh")
-    author.posts = [post]
-    categorization = Categorization.new
-    categorization.author = author
-
-    assert_predicate post_tagging_1, :persisted?
-    assert_predicate post_tagging_2, :persisted?
-    assert_predicate post, :persisted?
-    assert_predicate categorization, :new_record?
-    assert_predicate categorization.author, :new_record?
-    assert_queries_count(1) { assert_equal [post_tagging_1, post_tagging_2].sort, categorization.post_taggings.sort }
-  end
-
-  def test_setting_nested_has_many_through_many_association_on_new_record_sets_nested_through_records
-    account_1 = Account.create!(firm_name: "account 1", credit_limit: 100)
-    subscriber_1 = Subscriber.create!(nick: "nick 1", account: account_1)
-    account_2 = Account.create!(firm_name: "account 2", credit_limit: 100)
-    subscriber_2 = Subscriber.create!(nick: "nick 2", account: account_2)
-    subscription_1 = Subscription.new(subscriber: subscriber_1)
-    subscription_2 = Subscription.new(subscriber: subscriber_2)
-    book = Book.new
-    book.subscriptions = [subscription_1, subscription_2]
-
-    assert_predicate subscriber_1, :persisted?
-    assert_predicate subscriber_2, :persisted?
-    assert_predicate account_1, :persisted?
-    assert_predicate account_2, :persisted?
-    assert_predicate book, :new_record?
-    book.subscriptions.each { |subscription| assert_predicate subscription, :new_record? }
-    assert_no_queries { assert_equal [account_1, account_2].sort, book.subscriber_accounts.sort }
-  end
-
   def test_has_many_through_create_record
     assert books(:awdr).subscribers.create!(nick: "bob")
   end
@@ -1665,6 +1551,21 @@ def test_has_many_through_update_ids_with_conditions
     assert_equal [], author.nonspecial_categories_with_condition_ids
   end
 
+  def test_has_many_through_from_same_parent_to_same_child_creates_join_models
+    club = Club.new(name: "Awesome Rails Club")
+    member = club.simple_members.build(name: "Jane Doe")
+
+    program = Program.new(name: "Learn Ruby on Rails")
+    program.members << member
+
+    club.programs << program
+
+    club.save!
+
+    assert_equal(1, program.enrollments.size)
+    assert_equal(1, club.simple_memberships.size)
+  end
+
   def test_single_has_many_through_association_with_unpersisted_parent_instance
     post_with_single_has_many_through = Class.new(Post) do
       def self.name; "PostWithSingleHasManyThrough"; end

activerecord/test/cases/associations/has_one_through_associations_test.rb

@@ -23,7 +23,6 @@
 require "models/shop_account"
 require "models/customer_carrier"
 require "models/cpk"
-require "models/rating"
 
 class HasOneThroughAssociationsTest < ActiveRecord::TestCase
   fixtures :member_types, :members, :clubs, :memberships, :sponsors, :organizations, :minivans,
@@ -44,63 +43,6 @@ def test_has_one_through_executes_limited_query
     end
   end
 
-  def test_setting_association_on_new_record_sets_through_record
-    club = Club.create!
-    membership = CurrentMembership.new(club: club)
-    member = Member.new
-    member.current_membership = membership
-
-    assert_predicate club, :persisted?
-    assert_predicate member, :new_record?
-    assert_predicate member.current_membership, :new_record?
-    assert_no_queries { assert_equal club, member.club }
-  end
-
-  def test_setting_association_on_new_record_sets_nested_through_record
-    category = Category.create!(name: "General")
-    club = Club.create!(category: category)
-    membership = CurrentMembership.new(club: club)
-    member = Member.new
-    member.current_membership = membership
-
-    assert_predicate category, :persisted?
-    assert_predicate club, :persisted?
-    assert_predicate member, :new_record?
-    assert_predicate member.current_membership, :new_record?
-    assert_no_queries { assert_equal club.category, member.club_category }
-  end
-
-  def test_setting_association_on_new_record_sets_not_loaded_nested_through_record
-    category = Category.create!(name: "General")
-    club = Club.create!(category: category)
-    club.reload
-    membership = CurrentMembership.new(club: club)
-    member = Member.new
-    member.current_membership = membership
-
-    assert_predicate category, :persisted?
-    assert_predicate club, :persisted?
-    assert_predicate member, :new_record?
-    assert_predicate member.current_membership, :new_record?
-    assert_queries_count(1) { assert_equal club.category, member.club_category }
-  end
-
-  def test_setting_association_on_new_record_sets_nested_through_record_with_belongs_to
-    essay = Essay.create!
-    author = Author.create!(name: "Josh", owned_essay: essay)
-    post = Post.create!(title: "Foo", body: "Bar", author: author)
-    comment = Comment.new(post: post)
-    rating = Rating.new
-    rating.comment = comment
-
-    assert_predicate essay, :persisted?
-    assert_predicate author, :persisted?
-    assert_predicate post, :persisted?
-    assert_predicate rating, :new_record?
-    assert_predicate rating.comment, :new_record?
-    assert_no_queries { assert_equal essay, rating.owned_essay }
-  end
-
   def test_creating_association_creates_through_record
     new_member = Member.create(name: "Chris")
     new_member.club = Club.create(name: "LRUG")

activerecord/test/cases/autosave_association_test.rb

@@ -40,9 +40,6 @@
 require "models/cake_designer"
 require "models/drink_designer"
 require "models/cpk"
-require "models/family"
-require "models/family_tree"
-require "models/user"
 
 class TestAutosaveAssociationsInGeneral < ActiveRecord::TestCase
   def test_autosave_works_even_when_other_callbacks_update_the_parent_model
@@ -1150,34 +1147,6 @@ def test_autosave_new_record_with_after_create_callback_and_habtm_association
 
     assert_equal 1, post.categories.reload.length
   end
-
-  FamilyLoadingMiddleAndThroughRecordsBeforeSave = Class.new(Family) do
-    before_save do
-      family_trees.map(&:member) + members
-    end
-  end
-
-  def test_autosave_new_record_with_hmt_and_middle_record_built_by_parent
-    family = FamilyLoadingMiddleAndThroughRecordsBeforeSave.new
-    family_tree = family.family_trees.build
-    family_tree.build_member
-    family.save!
-    family.reload
-
-    assert_equal 1, family.family_trees.size
-    assert_equal 1, family.members.size
-  end
-
-  def test_autosave_new_record_with_hmt_and_middle_record_built_by_through_record
-    family = FamilyLoadingMiddleAndThroughRecordsBeforeSave.new
-    member = family.members.build
-    family.family_trees.build(member: member)
-    family.save!
-    family.reload
-
-    assert_equal 1, family.family_trees.size
-    assert_equal 1, family.members.size
-  end
 end
 
 class TestDestroyAsPartOfAutosaveAssociation < ActiveRecord::TestCase

activerecord/test/models/book.rb

@@ -9,7 +9,6 @@ class Book < ActiveRecord::Base
 
   has_many :subscriptions
   has_many :subscribers, through: :subscriptions
-  has_many :subscriber_accounts, through: :subscribers, source: :account
 
   has_one :essay
 

activerecord/test/models/club.rb

@@ -15,6 +15,12 @@ class Club < ActiveRecord::Base
 
   scope :general, -> { left_joins(:category).where(categories: { name: "General" }).unscope(:limit) }
 
+  has_many :program_offerings
+  has_many :programs, through: :program_offerings
+
+  has_many :simple_memberships, class_name: "Membership"
+  has_many :simple_members, through: :simple_memberships, foreign_key: "member_id"
+
   accepts_nested_attributes_for :membership
 
   private

activerecord/test/models/enrollment.rb

@@ -0,0 +1,6 @@
+# frozen_string_literal: true
+
+class Enrollment < ActiveRecord::Base
+  belongs_to :program
+  belongs_to :member, class_name: "SimpleMember"
+end