Don't skip in-memory insertion of associations when loaded in validate

This was caused by 6d0d83a3. While the
bug it's trying to fix is handled if the association is loaded in an
after_(create|save) callback, it doesn't handle any cases that load the
association before the persistence takes place (validation, or before_*
filters). Instead of caring about the timing of persistence, we can just
ensure that we're not double adding the record instead.

The test from that commit actually broke, but it was not because the bug
has been re-introduced. It was because `Bulb` in our test suite is doing
funky things that look like STI but isn't STI, so equality comparison
didn't happen as the loaded model was of a different class.

Fixes #26661.

activerecord/lib/active_record/associations/collection_association.rb

@@ -426,15 +426,12 @@ def add_to_target(record, skip_callbacks = false, &block)
       def replace_on_target(record, index, skip_callbacks)
         callback(:before_add, record) unless skip_callbacks
 
-        was_loaded = loaded?
         yield(record) if block_given?
 
-        unless !was_loaded && loaded?
-          if index
-            @target[index] = record
-          else
-            @target << record
-          end
+        if index
+          @target[index] = record
+        else
+          append_record(record)
         end
 
         callback(:after_add, record) unless skip_callbacks
@@ -657,6 +654,10 @@ def first_nth_or_last(type, *args)
             set_inverse_instance record if record.is_a? ActiveRecord::Base
           end
         end
+
+        def append_record(record)
+          @target << record unless @target.include?(record)
+        end
     end
   end
 end

activerecord/lib/active_record/associations/has_many_through_association.rb

@@ -203,6 +203,10 @@ def find_target
         def invertible_for?(record)
           false
         end
+
+        def append_record(record)
+          @target << record
+        end
     end
   end
 end

activerecord/test/cases/associations/has_many_associations_test.rb

@@ -2390,10 +2390,13 @@ def self.name
     assert_equal [first_bulb, second_bulb], car.bulbs
   end
 
-  test 'double insertion of new object to association when same association used in the after create callback of a new object' do
-    car = Car.create!
-    car.bulbs << TrickyBulb.new
-    assert_equal 1, car.bulbs.size
+  test "double insertion of new object to association when same association used in the after create callback of a new object" do
+    reset_callbacks(:save, Bulb) do
+      Bulb.after_save { |record| record.car.bulbs.to_a }
+      car = Car.create!
+      car.bulbs << Bulb.new
+      assert_equal 1, car.bulbs.size
+    end
   end
 
   def test_association_force_reload_with_only_true_is_deprecated
@@ -2439,4 +2442,31 @@ def test_ids_reader_memoization
     assert_equal [bulb.id], car.bulb_ids
     assert_no_queries { car.bulb_ids }
   end
+
+  def test_loading_association_in_validate_callback_doesnt_affect_persistence
+    reset_callbacks(:validation, Bulb) do
+      Bulb.after_validation { |m| m.car.bulbs.load }
+
+      car = Car.create!(name: "Car")
+      bulb = car.bulbs.create!
+
+      assert_equal [bulb], car.bulbs
+    end
+  end
+
+  private
+
+    def reset_callbacks(kind, klass)
+      old_callbacks = {}
+      old_callbacks[klass] = klass.send("_#{kind}_callbacks").dup
+      klass.subclasses.each do |subclass|
+        old_callbacks[subclass] = subclass.send("_#{kind}_callbacks").dup
+      end
+      yield
+    ensure
+      klass.send("_#{kind}_callbacks=", old_callbacks[klass])
+      klass.subclasses.each do |subclass|
+        subclass.send("_#{kind}_callbacks=", old_callbacks[subclass])
+      end
+    end
 end

activerecord/test/models/bulb.rb

@@ -50,9 +50,3 @@ class FailedBulb < Bulb
     throw(:abort)
   end
 end
-
-class TrickyBulb < Bulb
-  after_create do |record|
-    record.car.bulbs.to_a
-  end
-end