Skip to content

Commit

Permalink
Fix #hash and #== for composite pk models
Browse files Browse the repository at this point in the history
Given a model with a composite primary key, for example:
`TravelRoute.primary_key = [:from, :to]`

and two objects of the given model, objects `a` and `b`, should be
equal to each other and have the same `hash` value if they have the same
values for the composite primary key, like:

```ruby
a = TravelRoute.new(from: "NYC", to: "LAX")
b = TravelRoute.new(from: "NYC", to: "LAX")
a == b # => true

a.hash == b.hash # => true
```

At the same time, two objects of the given model should not be equal to
each other and should have different `hash` values if they have
different primary key values or no primary key being set, like:

```ruby
a = TravelRoute.new(from: "NYC", to: "LAX")
b = TravelRoute.new(from: "NYC", to: "SFO")
a == b # => false
a.hash == b.hash # => false

TravelRoute.new == TravelRoute.new # => false
TravelRoute.new.hash == TravelRoute.new.hash # => false
```
  • Loading branch information
nvasilevski committed Mar 22, 2023
1 parent 3827576 commit 7b1a617
Show file tree
Hide file tree
Showing 4 changed files with 62 additions and 2 deletions.
Expand Up @@ -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)
_write_attribute(@primary_key, value)
Expand Down
4 changes: 2 additions & 2 deletions activerecord/lib/active_record/core.rb
Expand Up @@ -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? :==
Expand All @@ -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
Expand Down
37 changes: 37 additions & 0 deletions activerecord/test/cases/core_test.rb
Expand Up @@ -4,6 +4,7 @@
require "models/person"
require "models/topic"
require "pp"
require "models/cpk"

class NonExistentTable < ActiveRecord::Base; end

Expand Down Expand Up @@ -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
17 changes: 17 additions & 0 deletions activerecord/test/cases/primary_keys_test.rb
Expand Up @@ -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]
Expand Down Expand Up @@ -411,6 +418,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
Expand Down

0 comments on commit 7b1a617

Please sign in to comment.