Merge pull request rails#49625 from Shopify/disallow-association-fk-as-an-array

Raise on `foreign_key:` being passed as an array in associations

Author: rafaelfranca

activerecord/CHANGELOG.md

@@ -1,3 +1,7 @@
+*   Raise on `foreign_key:` being passed as an array in associations
+
+    *Nikita Vasilevsky*
+
 *   Ensure `#signed_id` outputs `url_safe` strings.
 
     *Jason Meller*

activerecord/lib/active_record/reflection.rb

@@ -382,6 +382,7 @@ def initialize(name, scope, options, active_record)
         @klass         = options[:anonymous_class]
         @plural_name   = active_record.pluralize_table_names ?
                             name.to_s.pluralize : name.to_s
+        validate_reflection!
       end
 
       def autosave=(autosave)
@@ -433,6 +434,17 @@ def scope_for(relation, owner = nil)
         def derive_class_name
           name.to_s.camelize
         end
+
+        def validate_reflection!
+          return unless options[:foreign_key].is_a?(Array)
+
+          message = <<~MSG.squish
+            Passing #{options[:foreign_key]} array to :foreign_key option
+            on the #{active_record}##{name} association is not supported.
+            Use the query_constraints: #{options[:foreign_key]} option instead to represent a composite foreign key.
+          MSG
+          raise ArgumentError, message
+        end
     end
 
     # Holds all the metadata about an aggregation as it was specified in the

activerecord/test/cases/associations_test.rb

@@ -194,6 +194,30 @@ def test_querying_by_relation_with_composite_key
     assert_equal(expected_posts.map(&:id).sort, blog_posts.map(&:id).sort)
   end
 
+  def test_has_many_with_foreign_key_as_an_array_raises
+    expected_message = <<~MSG.squish
+      Passing [:blog_id, :blog_post_id] array to :foreign_key option
+      on the Sharded::BlogPost#broken_array_fk_comments association is not supported.
+      Use the query_constraints: [:blog_id, :blog_post_id] option instead to represent a composite foreign key.
+    MSG
+    assert_raises ArgumentError, match: expected_message do
+      Sharded::BlogPost.has_many :broken_array_fk_comments,
+        class_name: "Sharded::Comment", foreign_key: [:blog_id, :blog_post_id]
+    end
+  end
+
+  def test_belongs_to_with_foreign_key_as_an_array_raises
+    expected_message = <<~MSG.squish
+      Passing [:blog_id, :blog_post_id] array to :foreign_key option
+      on the Sharded::Comment#broken_array_fk_blog_post association is not supported.
+      Use the query_constraints: [:blog_id, :blog_post_id] option instead to represent a composite foreign key.
+    MSG
+    assert_raises ArgumentError, match: expected_message do
+      Sharded::Comment.belongs_to :broken_array_fk_blog_post,
+        class_name: "Sharded::Blog", foreign_key: [:blog_id, :blog_post_id]
+    end
+  end
+
   def test_has_many_association_with_composite_foreign_key_loads_records
     blog_post = sharded_blog_posts(:great_post_blog_one)
 

activerecord/test/cases/reflection_test.rb

@@ -226,6 +226,17 @@ def test_has_many_reflection
     assert_equal "companies", Firm.reflect_on_association(:clients_of_firm).table_name
   end
 
+  def test_has_many_reflection_with_array_fk_raises
+    expected_message = <<~MSG.squish
+      Passing [:firm_id, :firm_name] array to :foreign_key option
+      on the Firm#clients association is not supported.
+      Use the query_constraints: [:firm_id, :firm_name] option instead to represent a composite foreign key.
+    MSG
+    assert_raises ArgumentError, match: expected_message do
+      ActiveRecord::Reflection.create(:has_many, :clients, nil, { foreign_key: [:firm_id, :firm_name] }, Firm)
+    end
+  end
+
   def test_has_one_reflection
     reflection_for_account = ActiveRecord::Reflection.create(:has_one, :account, nil, { foreign_key: "firm_id", dependent: :destroy }, Firm)
     assert_equal reflection_for_account, Firm.reflect_on_association(:account)
@@ -234,6 +245,17 @@ def test_has_one_reflection
     assert_equal "accounts", Firm.reflect_on_association(:account).table_name
   end
 
+  def has_one_reflection_with_array_fk_raises
+    expected_message = <<~MSG.squish
+      Passing [:firm_id, :firm_name] array to :foreign_key option
+      on the Firm#account association is not supported.
+      Use the query_constraints: [:firm_id, :firm_name] option instead to represent a composite foreign key.
+    MSG
+    assert_raises ArgumentError, match: expected_message do
+      ActiveRecord::Reflection.create(:has_one, :account, nil, { foreign_key: [:firm_id, :firm_name] }, Firm)
+    end
+  end
+
   def test_belongs_to_inferred_foreign_key_from_assoc_name
     Company.belongs_to :foo
     assert_equal "foo_id", Company.reflect_on_association(:foo).foreign_key
@@ -243,6 +265,17 @@ def test_belongs_to_inferred_foreign_key_from_assoc_name
     assert_equal "xyzzy_id", Company.reflect_on_association(:baz).foreign_key
   end
 
+  def test_belongs_to_reflection_with_array_fk_raises
+    expected_message = <<~MSG.squish
+      Passing [:firm_id, :firm_name] array to :foreign_key option
+      on the Firm#client association is not supported.
+      Use the query_constraints: [:firm_id, :firm_name] option instead to represent a composite foreign key.
+    MSG
+    assert_raises ArgumentError, match: expected_message do
+      ActiveRecord::Reflection.create(:belongs_to, :client, nil, { foreign_key: [:firm_id, :firm_name] }, Firm)
+    end
+  end
+
   def test_association_reflection_in_modules
     ActiveRecord::Base.store_full_sti_class = false