Merge pull request #47864 from zenspider/zenspider/ar_core_hash

Fix AR#== and AR#hash when new_record?

activerecord/lib/active_record/core.rb

@@ -551,10 +551,12 @@ def encode_with(coder)
     # Note also that destroying a record preserves its ID in the model instance, so deleted
     # models are still comparable.
     def ==(comparison_object)
+      return super if new_record?
       super ||
         comparison_object.instance_of?(self.class) &&
         primary_key_values_present? &&
-        comparison_object.id == id
+        comparison_object.id == id &&
+        !comparison_object.new_record?
     end
     alias :eql? :==
 
@@ -563,8 +565,8 @@ def ==(comparison_object)
     def hash
       id = self.id
 
-      if primary_key_values_present?
-        self.class.hash ^ id.hash
+      if primary_key_values_present? && !new_record?
+        [self.class, id].hash
       else
         super
       end

activerecord/test/cases/attribute_methods_test.rb

@@ -811,8 +811,10 @@ def topic.approved; false; end
 
   test "YAML dumping a record with time zone-aware attribute" do
     in_time_zone "Pacific Time (US & Canada)" do
+      Topic.where(id: 1).delete_all
       record = Topic.new(id: 1)
       record.written_on = "Jan 01 00:00:00 2014"
+      record.save!
       payload = YAML.dump(record)
       assert_equal record, YAML.respond_to?(:unsafe_load) ? YAML.unsafe_load(payload) : YAML.load(payload)
     end

activerecord/test/cases/base_test.rb

@@ -624,12 +624,6 @@ def test_equality_of_destroyed_records
     assert_equal topic_2, topic_1
   end
 
-  def test_equality_with_blank_ids
-    one = Subscriber.new(id: "")
-    two = Subscriber.new(id: "")
-    assert_equal one, two
-  end
-
   def test_equality_of_relation_and_collection_proxy
     car = Car.create!
     car.bulbs.build

activerecord/test/cases/core_test.rb

@@ -7,9 +7,28 @@
 require "models/cpk"
 
 class NonExistentTable < ActiveRecord::Base; end
+class PkWithDefault < ActiveRecord::Base; end
 
 class CoreTest < ActiveRecord::TestCase
-  fixtures :topics
+  fixtures :topics, :cpk_books
+
+  def test_eql_on_default_pk
+    saved_record = PkWithDefault.new
+    saved_record.save!
+    assert_equal 123, saved_record.id
+
+    record = PkWithDefault.new
+    assert_equal 123, record.id
+
+    record2 = PkWithDefault.new
+    assert_equal 123, record2.id
+
+    assert     record.eql?(record),       "record should eql? itself"
+    assert_not record.eql?(saved_record), "new record should not eql? saved"
+    assert_not saved_record.eql?(record), "saved record should not eql? new"
+    assert_not record.eql?(record2),      "new record should not eql? new record"
+    assert_not record2.eql?(record),      "new record should not eql? new record"
+  end
 
   def test_inspect_class
     assert_equal "ActiveRecord::Base", ActiveRecord::Base.inspect
@@ -155,12 +174,12 @@ def test_find_by_cache_does_not_duplicate_entries
 
   def test_composite_pk_models_added_to_a_set
     library = Set.new
-    # with primary key present
+    # new record 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)
+    library << cpk_books(:cpk_great_author_first_book)
+    library << cpk_books(:cpk_great_author_first_book)
 
     # without primary key being set
     library << Cpk::Book.new(title: "Book A")
@@ -170,22 +189,34 @@ def test_composite_pk_models_added_to_a_set
   end
 
   def test_composite_pk_models_equality
-    assert Cpk::Book.new(author_id: 1, number: 2) == Cpk::Book.new(author_id: 1, number: 2)
+    book = cpk_books(:cpk_great_author_first_book)
+    book_instance_1 = Cpk::Book.find_by(author_id: book.author_id, number: book.number)
+    book_instance_2 = Cpk::Book.find_by(author_id: book.author_id, number: book.number)
+
+    assert book_instance_1 == book_instance_1
+    assert book_instance_1 == book_instance_2
 
-    assert_not Cpk::Book.new(author_id: 1, number: 2) == Cpk::Book.new(author_id: 1, number: 3)
+    # two new records with the same primary key
+    assert_not Cpk::Book.new(author_id: 1, number: 2) == Cpk::Book.new(author_id: 1, number: 2)
+    # two new records with an empty primary key values
     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")
+    # two persisted records with a different primary key
+    assert_not cpk_books(:cpk_great_author_first_book) == cpk_books(:cpk_great_author_second_book)
   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
+    book = cpk_books(:cpk_great_author_first_book)
+    book_instance_1 = Cpk::Book.find_by(author_id: book.author_id, number: book.number)
+    book_instance_2 = Cpk::Book.find_by(author_id: book.author_id, number: book.number)
+
+    assert_equal book_instance_1.hash, book_instance_1.hash
+    assert_equal book_instance_1.hash, book_instance_2.hash
 
-    assert_not_equal Cpk::Book.new(author_id: 1, number: 2).hash, Cpk::Book.new(author_id: 1, number: 3).hash
+    # two new records with the same primary key
+    assert_not_equal Cpk::Book.new(author_id: 1, number: 2).hash, Cpk::Book.new(author_id: 1, number: 2).hash
+    # two new records with an empty primary key values
     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
+    # two persisted records with a different primary key
+    assert_not_equal cpk_books(:cpk_great_author_first_book).hash, cpk_books(:cpk_great_author_second_book).hash
   end
 end

activerecord/test/cases/filter_attributes_test.rb

@@ -68,6 +68,7 @@ class FilterAttributesTest < ActiveRecord::TestCase
     ActiveRecord::Base.filter_attributes = [ lambda { |key, value| value.reverse! if key == "name" } ]
     account = Admin::Account.new(id: 123, name: "37signals")
     account.inspect
+    account.save!
     assert_equal account, Marshal.load(Marshal.dump(account))
   end
 

activerecord/test/schema/schema.rb

@@ -1392,6 +1392,10 @@
     t.bigint :toooooooo_long_a_id, null: false
     t.bigint :toooooooo_long_b_id, null: false
   end
+
+  create_table :pk_with_defaults, id: false, force: true do |t|
+    t.bigint :id, default: 123
+  end
 end
 
 Course.connection.create_table :courses, force: true do |t|