Merge pull request #47731 from Shopify/fix-CPK-models-equality

Fix `#hash` and `#==` for composite pk models

activerecord/lib/active_record/attribute_methods/primary_key.rb

@@ -21,6 +21,12 @@ def id
         @primary_key.map { |pk| _read_attribute(pk) }
       end
 
+      def primary_key_values_present? # :nodoc:
+        return id.all? if self.class.composite_primary_key?
+
+        !!id
+      end
+
       # Sets the primary key column's value.
       def id=(value)
         if self.class.composite_primary_key?

activerecord/lib/active_record/core.rb

@@ -549,7 +549,7 @@ def encode_with(coder)
     def ==(comparison_object)
       super ||
         comparison_object.instance_of?(self.class) &&
-        !id.nil? &&
+        primary_key_values_present? &&
         comparison_object.id == id
     end
     alias :eql? :==
@@ -559,7 +559,7 @@ def ==(comparison_object)
     def hash
       id = self.id
 
-      if id
+      if primary_key_values_present?
         self.class.hash ^ id.hash
       else
         super

activerecord/test/cases/core_test.rb

@@ -4,6 +4,7 @@
 require "models/person"
 require "models/topic"
 require "pp"
+require "models/cpk"
 
 class NonExistentTable < ActiveRecord::Base; end
 
@@ -151,4 +152,40 @@ def test_find_by_cache_does_not_duplicate_entries
       Topic.find_by(id: 1)
     end
   end
+
+  def test_composite_pk_models_added_to_a_set
+    library = Set.new
+    # with primary key present
+    library << Cpk::Book.new(author_id: 1, number: 2)
+
+    # duplicate
+    library << Cpk::Book.new(author_id: 1, number: 3)
+    library << Cpk::Book.new(author_id: 1, number: 3)
+
+    # without primary key being set
+    library << Cpk::Book.new(title: "Book A")
+    library << Cpk::Book.new(title: "Book B")
+
+    assert_equal 4, library.size
+  end
+
+  def test_composite_pk_models_equality
+    assert Cpk::Book.new(author_id: 1, number: 2) == Cpk::Book.new(author_id: 1, number: 2)
+
+    assert_not Cpk::Book.new(author_id: 1, number: 2) == Cpk::Book.new(author_id: 1, number: 3)
+    assert_not Cpk::Book.new == Cpk::Book.new
+    assert_not Cpk::Book.new(title: "Book A") == Cpk::Book.new(title: "Book B")
+    assert_not Cpk::Book.new(author_id: 1) == Cpk::Book.new(author_id: 1)
+    assert_not Cpk::Book.new(author_id: 1, title: "Same title") == Cpk::Book.new(author_id: 1, title: "Same title")
+  end
+
+  def test_composite_pk_models_hash
+    assert_equal Cpk::Book.new(author_id: 1, number: 2).hash, Cpk::Book.new(author_id: 1, number: 2).hash
+
+    assert_not_equal Cpk::Book.new(author_id: 1, number: 2).hash, Cpk::Book.new(author_id: 1, number: 3).hash
+    assert_not_equal Cpk::Book.new.hash, Cpk::Book.new.hash
+    assert_not_equal Cpk::Book.new(title: "Book A").hash, Cpk::Book.new(title: "Book B").hash
+    assert_not_equal Cpk::Book.new(author_id: 1).hash, Cpk::Book.new(author_id: 1).hash
+    assert_not_equal Cpk::Book.new(author_id: 1, title: "Same title").hash, Cpk::Book.new(author_id: 1, title: "Same title").hash
+  end
 end

activerecord/test/cases/primary_keys_test.rb

@@ -217,6 +217,13 @@ def composite_primary_key_is_false_for_a_non_cpk_model
     assert_not_predicate Dashboard, :composite_primary_key?
   end
 
+  def test_primary_key_values_present
+    assert_predicate Topic.new(id: 1), :primary_key_values_present?
+
+    assert_not_predicate Topic.new, :primary_key_values_present?
+    assert_not_predicate Topic.new(title: "Topic A"), :primary_key_values_present?
+  end
+
   if current_adapter?(:PostgreSQLAdapter)
     def test_serial_with_quoted_sequence_name
       column = MixedCaseMonkey.columns_hash[MixedCaseMonkey.primary_key]
@@ -427,6 +434,16 @@ def test_id_is_not_defined_on_a_model_with_composite_primary_key
   def composite_primary_key_is_true_for_a_cpk_model
     assert_predicate Cpk::Book, :composite_primary_key?
   end
+
+  def test_primary_key_values_present_for_a_composite_pk_model
+    assert_predicate Cpk::Book.new(author_id: 1, number: 1), :primary_key_values_present?
+
+    assert_not_predicate Cpk::Book.new, :primary_key_values_present?
+    assert_not_predicate Cpk::Book.new(author_id: 1), :primary_key_values_present?
+    assert_not_predicate Cpk::Book.new(number: 1), :primary_key_values_present?
+    assert_not_predicate Cpk::Book.new(title: "Book A"), :primary_key_values_present?
+    assert_not_predicate Cpk::Book.new(author_id: 1, title: "Book A"), :primary_key_values_present?
+  end
 end
 
 class PrimaryKeyIntegerNilDefaultTest < ActiveRecord::TestCase