From 678dd6ab805a2a39d685e066d089614d1845c720 Mon Sep 17 00:00:00 2001
From: Gannon McGibbon <gannon.mcgibbon@gmail.com>
Date: Tue, 12 May 2020 13:09:36 -0400
Subject: [PATCH] Refactor uncastable through reflection test to detect join
 key overrides

The uncastable through reflection check should be testing for foreign
key type overrides on the join model and not a non-integer type on the
through primary key.
---
 activerecord/lib/active_record/reflection.rb | 30 ++++++++++++++------
 activerecord/test/cases/reflection_test.rb   | 27 ++++++++++++++----
 2 files changed, 42 insertions(+), 15 deletions(-)

diff --git a/activerecord/lib/active_record/reflection.rb b/activerecord/lib/active_record/reflection.rb
index 9a0350d452..5c7f876b8a 100644
--- a/activerecord/lib/active_record/reflection.rb
+++ b/activerecord/lib/active_record/reflection.rb
@@ -771,15 +771,7 @@ module ActiveRecord
 
       def klass
         @klass ||= delegate_reflection.compute_class(class_name).tap do |klass|
-          if !parent_reflection.is_a?(HasAndBelongsToManyReflection) &&
-             !(klass.reflections.key?(options[:through].to_s) ||
-               klass.reflections.key?(options[:through].to_s.pluralize)) &&
-             active_record.type_for_attribute(active_record.primary_key).type != :integer
-            raise NotImplementedError, <<~MSG.squish
-              In order to correctly type cast #{active_record}.#{active_record.primary_key},
-              #{klass} needs to define a :#{options[:through]} association.
-            MSG
-          end
+          check_reflection_validity!(klass)
         end
       end
 
@@ -1003,6 +995,26 @@ module ActiveRecord
           options[:source_type] || source_reflection.class_name
         end
 
+        def custom_join_key_type?
+          source_reflection.active_record.attributes_to_define_after_schema_loads.key?(
+            delegate_reflection.foreign_key
+          )
+        end
+
+        def check_reflection_validity!(klass)
+          return unless custom_join_key_type?
+
+          through_reflection = options[:through].to_s
+
+          unless klass.reflections.key?(through_reflection) ||
+                 klass.reflections.key?(through_reflection.pluralize)
+            raise NotImplementedError, <<~MSG.squish
+              In order to correctly type cast #{active_record}.#{active_record.primary_key},
+              #{klass} needs to define a :#{through_reflection} association.
+            MSG
+          end
+        end
+
         delegate_methods = AssociationReflection.public_instance_methods -
           public_instance_methods
 
diff --git a/activerecord/test/cases/reflection_test.rb b/activerecord/test/cases/reflection_test.rb
index cc160a5d6e..1e141422f7 100644
--- a/activerecord/test/cases/reflection_test.rb
+++ b/activerecord/test/cases/reflection_test.rb
@@ -517,17 +517,33 @@ class ReflectionTest < ActiveRecord::TestCase
     end
 end
 
-class UncastableReflectionTest < ActiveRecord::TestCase
+class UncastableOverriddenAttributeReflectionTest < ActiveRecord::TestCase
+  class NickType < ActiveRecord::Type::Binary
+    def serialize(value)
+      super("nickname-#{value}")
+    end
+
+    def deserialize(value)
+      super(value).to_s.delete_prefix("nickname-")
+    end
+
+    def cast_value(value)
+      value.to_s
+    end
+  end
+
   class Book < ActiveRecord::Base
   end
 
   class Subscription < ActiveRecord::Base
     belongs_to :subscriber
     belongs_to :book
+    attribute :subscriber_id, NickType.new
   end
 
   class Subscriber < ActiveRecord::Base
     self.primary_key = "nick"
+    attribute :nick, NickType.new
     has_many :subscriptions
     has_one :subscription
     has_many :books, through: :subscriptions
@@ -550,17 +566,16 @@ class UncastableReflectionTest < ActiveRecord::TestCase
   test "uncastable has_many through: reflection" do
     error = assert_raises(NotImplementedError) { @subscriber.books }
     assert_equal <<~MSG.squish, error.message
-      In order to correctly type cast UncastableReflectionTest::Subscriber.nick,
-      UncastableReflectionTest::Book needs to define a :subscriptions association.
+      In order to correctly type cast #{self.class}::Subscriber.nick,
+      #{self.class}::Book needs to define a :subscriptions association.
     MSG
   end
 
   test "uncastable has_one through: reflection" do
     error = assert_raises(NotImplementedError) { @subscriber.book }
-
     assert_equal <<~MSG.squish, error.message
-      In order to correctly type cast UncastableReflectionTest::Subscriber.nick,
-      UncastableReflectionTest::Book needs to define a :subscription association.
+      In order to correctly type cast #{self.class}::Subscriber.nick,
+      #{self.class}::Book needs to define a :subscription association.
     MSG
   end