Merge pull request rails#51571 from Shopify/deprecate-explicit-query-constraints-in-favor-of-foreign-key

Warn about changing `query_constraints:` behavior

Author: eileencodes

activerecord/CHANGELOG.md

@@ -1,3 +1,7 @@
+*   Association option `query_constraints` is deprecated in favor of `foreign_key`.
+
+    *Nikita Vasilevsky*
+
 *   Add ENV["SKIP_TEST_DATABASE_TRUNCATE"] flag to speed up multi-process test runs on large DBs when all tests run within default txn. (This cuts ~10s from the test run of HEY when run by 24 processes against the 178 tables, since ~4,000 table truncates can then be skipped.)
 
     *DHH*

activerecord/lib/active_record/fixtures.rb

@@ -498,8 +498,8 @@ class FixtureClassNotFound < ActiveRecord::ActiveRecordError # :nodoc:
   #   # app/models/book_orders.rb
   #   class BookOrder < ApplicationRecord
   #     self.primary_key = [:shop_id, :id]
-  #     belongs_to :order, query_constraints: [:shop_id, :order_id]
-  #     belongs_to :book, query_constraints: [:author_id, :book_id]
+  #     belongs_to :order, foreign_key: [:shop_id, :order_id]
+  #     belongs_to :book, foreign_key: [:author_id, :book_id]
   #   end
   #
   # <code></code>

activerecord/lib/active_record/reflection.rb

@@ -522,6 +522,13 @@ def initialize(name, scope, options, active_record)
         @foreign_key = nil
         @association_foreign_key = nil
         @association_primary_key = nil
+        if options[:query_constraints]
+          ActiveRecord.deprecator.warn <<~MSG.squish
+            Setting `query_constraints:` option on `#{active_record}.#{macro} :#{name}` is deprecated.
+            To maintain current behavior, use the `foreign_key` option instead.
+          MSG
+        end
+
         # If the foreign key is an array, set query constraints options and don't use the foreign key
         if options[:foreign_key].is_a?(Array)
           options[:query_constraints] = options.delete(:foreign_key)

activerecord/test/cases/associations_test.rb

@@ -461,6 +461,28 @@ def test_nullify_composite_has_many_through_association
     assert_empty(blog_post.reload.tags)
     assert_not_predicate Sharded::BlogPostTag.where(blog_post_id: blog_post.id, blog_id: blog_post.blog_id), :exists?
   end
+
+  def test_using_query_constraints_warns_about_changing_behavior
+    has_many_expected_message = <<~MSG.squish
+      Setting `query_constraints:` option on `Sharded::BlogPost.has_many :qc_deprecated_comments` is deprecated.
+      To maintain current behavior, use the `foreign_key` option instead.
+    MSG
+
+    assert_deprecated(has_many_expected_message, ActiveRecord.deprecator) do
+      Sharded::BlogPost.has_many :qc_deprecated_comments,
+        class_name: "Sharded::Comment", query_constraints: [:blog_id, :blog_post_id]
+    end
+
+    belongs_to_expected_message = <<~MSG.squish
+      Setting `query_constraints:` option on `Sharded::Comment.belongs_to :qc_deprecated_blog_post` is deprecated.
+      To maintain current behavior, use the `foreign_key` option instead.
+    MSG
+
+    assert_deprecated(belongs_to_expected_message, ActiveRecord.deprecator) do
+      Sharded::Comment.belongs_to :qc_deprecated_blog_post,
+        class_name: "Sharded::Blog", query_constraints: [:blog_id, :blog_post_id]
+    end
+  end
 end
 
 class AssociationProxyTest < ActiveRecord::TestCase

activerecord/test/cases/reflection_test.rb

@@ -628,6 +628,35 @@ def test_belongs_to_reflection_with_query_constraints_infers_correct_foreign_key
     assert_equal ["blog_id", "blog_post_id"], blog_post_foreign_key
   end
 
+  def test_using_query_constraints_warns_about_changing_behavior
+    has_many_expected_message = <<~MSG.squish
+      Setting `query_constraints:` option on `Firm.has_many :clients` is deprecated.
+      To maintain current behavior, use the `foreign_key` option instead.
+    MSG
+
+    assert_deprecated(has_many_expected_message, ActiveRecord.deprecator) do
+      ActiveRecord::Reflection.create(:has_many, :clients, nil, { query_constraints: [:firm_id, :firm_name] }, Firm)
+    end
+
+    has_one_expected_message = <<~MSG.squish
+      Setting `query_constraints:` option on `Firm.has_one :account` is deprecated.
+      To maintain current behavior, use the `foreign_key` option instead.
+    MSG
+
+    assert_deprecated(has_one_expected_message, ActiveRecord.deprecator) do
+      ActiveRecord::Reflection.create(:has_one, :account, nil, { query_constraints: [:firm_id, :firm_name] }, Firm)
+    end
+
+    belongs_to_expected_message = <<~MSG.squish
+      Setting `query_constraints:` option on `Firm.belongs_to :client` is deprecated.
+      To maintain current behavior, use the `foreign_key` option instead.
+    MSG
+
+    assert_deprecated(belongs_to_expected_message, ActiveRecord.deprecator) do
+      ActiveRecord::Reflection.create(:belongs_to, :client, nil, { query_constraints: [:firm_id, :firm_name] }, Firm)
+    end
+  end
+
   private
     def assert_reflection(klass, association, options)
       assert reflection = klass.reflect_on_association(association)

activerecord/test/models/cpk/book.rb

@@ -5,11 +5,11 @@ class Book < ActiveRecord::Base
     attr_accessor :fail_destroy
 
     self.table_name = :cpk_books
-    belongs_to :order, autosave: true, query_constraints: [:shop_id, :order_id], counter_cache: true
-    belongs_to :order_explicit_fk_pk, class_name: "Cpk::Order", query_constraints: [:shop_id, :order_id], primary_key: [:shop_id, :id]
+    belongs_to :order, autosave: true, foreign_key: [:shop_id, :order_id], counter_cache: true
+    belongs_to :order_explicit_fk_pk, class_name: "Cpk::Order", foreign_key: [:shop_id, :order_id], primary_key: [:shop_id, :id]
     belongs_to :author, class_name: "Cpk::Author"
 
-    has_many :chapters, query_constraints: [:author_id, :book_id]
+    has_many :chapters, foreign_key: [:author_id, :book_id]
 
     before_destroy :prevent_destroy_if_set
 
@@ -27,17 +27,17 @@ class BrokenBook < Book
   end
 
   class BrokenBookWithNonCpkOrder < Book
-    belongs_to :order, class_name: "Cpk::NonCpkOrder", query_constraints: [:shop_id, :order_id]
+    belongs_to :order, class_name: "Cpk::NonCpkOrder", foreign_key: [:shop_id, :order_id]
   end
 
   class NonCpkBook < Book
     self.primary_key = :id
 
-    belongs_to :non_cpk_order, query_constraints: [:order_id]
+    belongs_to :non_cpk_order, foreign_key: [:order_id]
   end
 
   class NullifiedBook < Book
-    has_one :chapter, query_constraints: [:author_id, :book_id], dependent: :nullify
+    has_one :chapter, foreign_key: [:author_id, :book_id], dependent: :nullify
   end
 
   class BookWithOrderAgreements < Book

activerecord/test/models/cpk/chapter.rb

@@ -7,6 +7,6 @@ class Chapter < ActiveRecord::Base
     # to be shared between different databases
     self.primary_key = [:author_id, :id]
 
-    belongs_to :book, query_constraints: [:author_id, :book_id]
+    belongs_to :book, foreign_key: [:author_id, :book_id]
   end
 end

activerecord/test/models/cpk/comment.rb

@@ -3,7 +3,7 @@
 module Cpk
   class Comment < ActiveRecord::Base
     self.table_name = :cpk_comments
-    belongs_to :commentable, class_name: "Cpk::Post", query_constraints: %i[commentable_title commentable_author], polymorphic: true
-    belongs_to :post, class_name: "Cpk::Post", query_constraints: %i[commentable_title commentable_author]
+    belongs_to :commentable, class_name: "Cpk::Post", foreign_key: %i[commentable_title commentable_author], polymorphic: true
+    belongs_to :post, class_name: "Cpk::Post", foreign_key: %i[commentable_title commentable_author]
   end
 end

activerecord/test/models/cpk/order.rb

@@ -10,8 +10,8 @@ class Order < ActiveRecord::Base
     alias_attribute :id_value, :id
 
     has_many :order_agreements
-    has_many :books, query_constraints: [:shop_id, :order_id]
-    has_one :book, query_constraints: [:shop_id, :order_id]
+    has_many :books, foreign_key: [:shop_id, :order_id]
+    has_one :book, foreign_key: [:shop_id, :order_id]
     has_many :order_tags
     has_many :tags, through: :order_tags
   end
@@ -26,8 +26,8 @@ class BrokenOrder < Order
   class OrderWithSpecialPrimaryKey < Order
     self.primary_key = [:shop_id, :status]
 
-    has_many :books, query_constraints: [:shop_id, :status]
-    has_one :book, query_constraints: [:shop_id, :status]
+    has_many :books, foreign_key: [:shop_id, :status]
+    has_one :book, foreign_key: [:shop_id, :status]
   end
 
   class BrokenOrderWithNonCpkBooks < Order
@@ -46,7 +46,7 @@ class OrderWithPrimaryKeyAssociatedBook < Order
   end
 
   class OrderWithNullifiedBook < Order
-    has_one :book, query_constraints: [:shop_id, :order_id], dependent: :nullify
+    has_one :book, foreign_key: [:shop_id, :order_id], dependent: :nullify
   end
 
   class OrderWithSingularBookChapters < Order

activerecord/test/models/cpk/post.rb

@@ -3,6 +3,6 @@
 module Cpk
   class Post < ActiveRecord::Base
     self.table_name = :cpk_posts
-    has_many :comments, class_name: "Cpk::Comment", query_constraints: %i[commentable_title commentable_author], as: :commentable
+    has_many :comments, class_name: "Cpk::Comment", foreign_key: %i[commentable_title commentable_author], as: :commentable
   end
 end