Merge pull request rails#46439 from Shopify/use-primary-key-as-default-query-constraints

Configure `query_constraints_list` to use `primary_key` by default

Author: eileencodes

activerecord/lib/active_record/persistence.rb

@@ -1048,8 +1048,6 @@ def _find_record(options)
     end
 
     def _in_memory_query_constraints_hash
-      return { @primary_key => id } unless self.class.query_constraints_list
-
       self.class.query_constraints_list.index_with do |column_name|
         attribute(column_name)
       end
@@ -1061,8 +1059,6 @@ def apply_scoping?(options)
     end
 
     def _query_constraints_hash
-      return { @primary_key => id_in_database } unless self.class.query_constraints_list
-
       self.class.query_constraints_list.index_with do |column_name|
         attribute_in_database(column_name)
       end

activerecord/lib/active_record/query_constraints.rb

@@ -4,10 +4,6 @@ module ActiveRecord
   module QueryConstraints
     extend ActiveSupport::Concern
 
-    included do
-      class_attribute :query_constraints_list, instance_writer: false
-    end
-
     module ClassMethods
       # Accepts a list of attribute names to be used in the WHERE clause
       # of SELECT / UPDATE / DELETE queries.
@@ -39,8 +35,24 @@ module ClassMethods
       #   developer.reload
       #   # => SELECT "developers".* FROM "developers" WHERE "developers"."company_id" = 1 AND "developers"."id" = 1 LIMIT 1
       def query_constraints(*columns_list)
-        self.query_constraints_list = columns_list.map(&:to_s)
+        @_query_constraints_list = columns_list.map(&:to_s)
+      end
+
+      def query_constraints_list # :nodoc:
+        @_query_constraints_list ||= query_constraints_list_fallback
       end
+
+      private
+        # This is a fallback method that is used to determine the query_constraints_list
+        # for cases when the model is not explicitly configured with query_constraints.
+        # For a base class, just use the primary key.
+        # For a child class, use the primary key unless primary key was overridden.
+        # If the child's primary key was not overridden, use the parent's query_constraints_list.
+        def query_constraints_list_fallback # :nodoc:
+          return Array(primary_key) if base_class? || primary_key != base_class.primary_key
+
+          base_class.query_constraints_list
+        end
     end
   end
 end

activerecord/test/cases/persistence_test.rb

@@ -2,6 +2,8 @@
 
 require "cases/helper"
 require "models/aircraft"
+require "models/dashboard"
+require "models/clothing_item"
 require "models/post"
 require "models/comment"
 require "models/author"
@@ -19,7 +21,6 @@
 require "models/ship"
 require "models/admin"
 require "models/admin/user"
-require "models/clothing_item"
 
 class PersistenceTest < ActiveRecord::TestCase
   fixtures :topics, :companies, :developers, :accounts, :minimalistics, :authors, :author_addresses,
@@ -1396,3 +1397,49 @@ def test_it_is_possible_to_update_parts_of_the_query_constraints_config
     assert_equal("blue", ClothingItem.find_by(id: clothing_item.id).color)
   end
 end
+
+class QueryConstraintsTest < ActiveRecord::TestCase
+  fixtures :clothing_items, :dashboards, :topics, :posts
+
+  def test_primary_key_stays_the_same
+    assert_equal("id", ClothingItem.primary_key)
+  end
+
+  def test_query_constraints_uses_primary_key_by_default
+    post = posts(:welcome)
+    assert_uses_query_constraints_on_reload(post, "id")
+  end
+
+  def test_query_constraints_uses_manually_configured_primary_key
+    dashboard = dashboards(:cool_first)
+    assert_uses_query_constraints_on_reload(dashboard, "dashboard_id")
+  end
+
+  def test_child_overriden_primary_key_is_used_as_query_constraint
+    topic = topics(:first)
+    assert_uses_query_constraints_on_reload(topic, "id")
+
+    title_pk_topic = topic.becomes(TitlePrimaryKeyTopic)
+    title_pk_topic.author_name = "Nikita"
+
+    sql = capture_sql { title_pk_topic.save }.first
+    assert_match(/WHERE .*title/, sql)
+  end
+
+  def test_child_keeps_parents_query_constraints
+    clothing_item = clothing_items(:green_t_shirt)
+    assert_uses_query_constraints_on_reload(clothing_item, ["clothing_type", "color"])
+
+    used_clothing_item = clothing_items(:used_blue_jeans)
+    assert_uses_query_constraints_on_reload(used_clothing_item, ["clothing_type", "color"])
+  end
+
+  def assert_uses_query_constraints_on_reload(object, columns)
+    flunk("columns argument must not be empty") if columns.blank?
+
+    sql = capture_sql { object.reload }.first
+    Array(columns).each do |column|
+      assert_match(/WHERE .*#{column}/, sql)
+    end
+  end
+end

activerecord/test/cases/query_constraints_test.rb

@@ -2,13 +2,22 @@ green_pants:
   color: green
   clothing_type: pants
   description: Cool green pants
+  type: ClothingItem
 
 green_t_shirt:
   color: green
   clothing_type: t-shirt
   description: Cool green t-shirt
+  type: ClothingItem
 
 red_t_shirt:
   color: red
   clothing_type: t-shirt
   description: Cool red t-shirt
+  type: ClothingItem
+
+used_blue_jeans:
+  color: blue
+  clothing_type: pants
+  description: Cool blue jeans
+  type: ClothingItem::Used

activerecord/test/fixtures/clothing_items.yml

@@ -47,3 +47,4 @@ fifth:
   written_on: 2013-07-13t12:11:00.0099+01:00
   content: "--- Omakase\n...\n"
   approved: true
+

activerecord/test/fixtures/topics.yml

@@ -3,3 +3,6 @@
 class ClothingItem < ActiveRecord::Base
   query_constraints :clothing_type, :color
 end
+
+class ClothingItem::Used < ClothingItem
+end

activerecord/test/models/clothing_item.rb

@@ -236,6 +236,7 @@
   create_table :clothing_items, force: true do |t|
     t.string :clothing_type
     t.string :color
+    t.string :type
     t.text :description
 
     t.index [:clothing_type, :color], unique: true