From 9cadf6183507682d4b3faf3e1620c39c1f7fc765 Mon Sep 17 00:00:00 2001
From: Nikita Vasilevsky <nikita.vasilevsky@shopify.com>
Date: Tue, 9 Apr 2024 14:51:38 +0000
Subject: [PATCH] Warn about changing `query_constraints:` behavior

This commit adds a deprecation warning for the `query_constraints:`
association option. This option will change behavior in the future versions
of Rails and applications are encouraged to switch to `foreign_key:` to preserve the
current behavior.
---
 activerecord/CHANGELOG.md                     |  4 +++
 activerecord/lib/active_record/fixtures.rb    |  4 +--
 activerecord/lib/active_record/reflection.rb  |  7 +++++
 activerecord/test/cases/associations_test.rb  | 22 ++++++++++++++
 activerecord/test/cases/reflection_test.rb    | 29 +++++++++++++++++++
 activerecord/test/models/cpk/book.rb          | 12 ++++----
 activerecord/test/models/cpk/chapter.rb       |  2 +-
 activerecord/test/models/cpk/comment.rb       |  4 +--
 activerecord/test/models/cpk/order.rb         | 10 +++----
 activerecord/test/models/cpk/post.rb          |  2 +-
 activerecord/test/models/cpk/review.rb        |  2 +-
 .../models/sharded/blog_post_destroy_async.rb |  4 +--
 .../models/sharded/blog_post_with_revision.rb |  2 +-
 .../models/sharded/comment_destroy_async.rb   |  2 +-
 .../active_record_composite_primary_keys.md   | 10 +++----
 guides/source/association_basics.md           |  6 ++--
 16 files changed, 92 insertions(+), 30 deletions(-)

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index cd01fe1e9c43f..3828045c72be2 100644
--- a/activerecord/CHANGELOG.md
+++ b/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*
diff --git a/activerecord/lib/active_record/fixtures.rb b/activerecord/lib/active_record/fixtures.rb
index d73ab5b5d3821..04550daad1451 100644
--- a/activerecord/lib/active_record/fixtures.rb
+++ b/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>
diff --git a/activerecord/lib/active_record/reflection.rb b/activerecord/lib/active_record/reflection.rb
index 466f56d030194..60c7f3c7bbb77 100644
--- a/activerecord/lib/active_record/reflection.rb
+++ b/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)
diff --git a/activerecord/test/cases/associations_test.rb b/activerecord/test/cases/associations_test.rb
index c5a088371a084..6d3beeeb70feb 100644
--- a/activerecord/test/cases/associations_test.rb
+++ b/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
diff --git a/activerecord/test/cases/reflection_test.rb b/activerecord/test/cases/reflection_test.rb
index 3ea5c2ebd0714..60fcd20b77798 100644
--- a/activerecord/test/cases/reflection_test.rb
+++ b/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)
diff --git a/activerecord/test/models/cpk/book.rb b/activerecord/test/models/cpk/book.rb
index 0b3b20bae9462..10e73d933d3d0 100644
--- a/activerecord/test/models/cpk/book.rb
+++ b/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
diff --git a/activerecord/test/models/cpk/chapter.rb b/activerecord/test/models/cpk/chapter.rb
index 3ebfdb3699b12..2e1a9103224f2 100644
--- a/activerecord/test/models/cpk/chapter.rb
+++ b/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
diff --git a/activerecord/test/models/cpk/comment.rb b/activerecord/test/models/cpk/comment.rb
index bf02258d29bb6..5b98e11218db7 100644
--- a/activerecord/test/models/cpk/comment.rb
+++ b/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
diff --git a/activerecord/test/models/cpk/order.rb b/activerecord/test/models/cpk/order.rb
index 0e5085831be51..a0a8b733342f0 100644
--- a/activerecord/test/models/cpk/order.rb
+++ b/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
diff --git a/activerecord/test/models/cpk/post.rb b/activerecord/test/models/cpk/post.rb
index 4892465fa0ef9..b80c92ea1fedf 100644
--- a/activerecord/test/models/cpk/post.rb
+++ b/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
diff --git a/activerecord/test/models/cpk/review.rb b/activerecord/test/models/cpk/review.rb
index 1fa7b73a9cc01..1717802f79e79 100644
--- a/activerecord/test/models/cpk/review.rb
+++ b/activerecord/test/models/cpk/review.rb
@@ -4,6 +4,6 @@ module Cpk
   class Review < ActiveRecord::Base
     self.table_name = :cpk_reviews
 
-    belongs_to :book, class_name: "Cpk::Book", query_constraints: [:author_id, :number]
+    belongs_to :book, class_name: "Cpk::Book", foreign_key: [:author_id, :number]
   end
 end
diff --git a/activerecord/test/models/sharded/blog_post_destroy_async.rb b/activerecord/test/models/sharded/blog_post_destroy_async.rb
index ae6387459d2b8..0e4f6651084d3 100644
--- a/activerecord/test/models/sharded/blog_post_destroy_async.rb
+++ b/activerecord/test/models/sharded/blog_post_destroy_async.rb
@@ -6,9 +6,9 @@ class BlogPostDestroyAsync < ActiveRecord::Base
     query_constraints :blog_id, :id
 
     belongs_to :blog
-    has_many :comments, dependent: :destroy_async, query_constraints: [:blog_id, :blog_post_id], class_name: "Sharded::CommentDestroyAsync"
+    has_many :comments, dependent: :destroy_async, foreign_key: [:blog_id, :blog_post_id], class_name: "Sharded::CommentDestroyAsync"
 
-    has_many :blog_post_tags, query_constraints: [:blog_id, :blog_post_id], class_name: "Sharded::BlogPostTag"
+    has_many :blog_post_tags, foreign_key: [:blog_id, :blog_post_id], class_name: "Sharded::BlogPostTag"
     has_many :tags, through: :blog_post_tags, dependent: :destroy_async, class_name: "Sharded::Tag"
   end
 end
diff --git a/activerecord/test/models/sharded/blog_post_with_revision.rb b/activerecord/test/models/sharded/blog_post_with_revision.rb
index c86b571955be5..1c947cdcab4e7 100644
--- a/activerecord/test/models/sharded/blog_post_with_revision.rb
+++ b/activerecord/test/models/sharded/blog_post_with_revision.rb
@@ -6,6 +6,6 @@ class BlogPostWithRevision < ActiveRecord::Base
     self.table_name = :sharded_blog_posts
     query_constraints :blog_id, :revision, :id
 
-    has_many :comments, primary_key: [:blog_id, :id], query_constraints: [:blog_id, :blog_post_id]
+    has_many :comments, primary_key: [:blog_id, :id], foreign_key: [:blog_id, :blog_post_id]
   end
 end
diff --git a/activerecord/test/models/sharded/comment_destroy_async.rb b/activerecord/test/models/sharded/comment_destroy_async.rb
index a50036af0f2cc..69f04a5d9dcf0 100644
--- a/activerecord/test/models/sharded/comment_destroy_async.rb
+++ b/activerecord/test/models/sharded/comment_destroy_async.rb
@@ -5,7 +5,7 @@ class CommentDestroyAsync < ActiveRecord::Base
     self.table_name = :sharded_comments
     query_constraints :blog_id, :id
 
-    belongs_to :blog_post, dependent: :destroy_async, query_constraints: [:blog_id, :blog_post_id], class_name: "Sharded::BlogPostDestroyAsync"
+    belongs_to :blog_post, dependent: :destroy_async, foreign_key: [:blog_id, :blog_post_id], class_name: "Sharded::BlogPostDestroyAsync"
     belongs_to :blog_post_by_id, class_name: "Sharded::BlogPostDestroyAsync", foreign_key: :blog_post_id
     belongs_to :blog
   end
diff --git a/guides/source/active_record_composite_primary_keys.md b/guides/source/active_record_composite_primary_keys.md
index d4e681034b132..1e09092c026a1 100644
--- a/guides/source/active_record_composite_primary_keys.md
+++ b/guides/source/active_record_composite_primary_keys.md
@@ -158,7 +158,7 @@ SELECT * FROM orders WHERE id = 2
 
 This only works if the model's composite primary key contains the `:id` column,
 _and_ the column is unique for all records. In order to use the full composite
-primary key in associations, set the `query_constraints` option on the
+primary key in associations, set the `foreign_key:` option on the
 association. This option specifies a composite foreign key on the association,
 meaning that all columns in the foreign key will be used to query the
 associated record(s). For example:
@@ -166,11 +166,11 @@ associated record(s). For example:
 ```ruby
 class Author < ApplicationRecord
   self.primary_key = [:first_name, :last_name]
-  has_many :books, query_constraints: [:first_name, :last_name]
+  has_many :books, foreign_key: [:first_name, :last_name]
 end
 
 class Book < ApplicationRecord
-  belongs_to :author, query_constraints: [:author_first_name, :author_last_name]
+  belongs_to :author, foreign_key: [:author_first_name, :author_last_name]
 end
 ```
 
@@ -285,8 +285,8 @@ you must use the `composite_identify` method:
 ```ruby
 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
 ```
 
diff --git a/guides/source/association_basics.md b/guides/source/association_basics.md
index bd27435d7e5c7..13de54f91d883 100644
--- a/guides/source/association_basics.md
+++ b/guides/source/association_basics.md
@@ -579,18 +579,18 @@ SELECT * FROM orders WHERE id = 2
 ```
 
 This only works if the model's composite primary key contains the `:id` column, _and_ the column is unique for
-all records. In order to use the full composite primary key in associations, set the `query_constraints` option on
+all records. In order to use the full composite primary key in associations, set the `foreign_key:` option on
 the association. This option specifies a composite foreign key on the association: all columns in the foreign key will
 be used when querying the associated record(s). For example:
 
 ```ruby
 class Author < ApplicationRecord
   self.primary_key = [:first_name, :last_name]
-  has_many :books, query_constraints: [:first_name, :last_name]
+  has_many :books, foreign_key: [:first_name, :last_name]
 end
 
 class Book < ApplicationRecord
-  belongs_to :author, query_constraints: [:author_first_name, :author_last_name]
+  belongs_to :author, foreign_key: [:author_first_name, :author_last_name]
 end
 ```