Skip to content

Commit

Permalink
Support the use of primary keys other than "id" in ActiveRecord.
Browse files Browse the repository at this point in the history
Previously the diffing code made an assumption that "id" would always be
the primary field. This is not always the case, and the use of
read_attribute(:id) is deprecated in rails/rails@39997a0.

This changes adds support for use of custom primary key fields, and
updates the Person test model to use the "person_id" primary key.

Note that this will almost certainly still run into issues if the model
uses the newly introduced composite primary key type, so this would
require additional changes outside the scope of this fix.
  • Loading branch information
benk-gc committed Apr 1, 2024
1 parent caa72ab commit cb9b75c
Show file tree
Hide file tree
Showing 7 changed files with 31 additions and 14 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@ def self.applies_to?(value)
value.is_a?(::ActiveRecord::Base)
end

def id
object.class.primary_key
end

def call
Core::InspectionTree.new do |t1|
t1.as_lines_when_rendering_to_lines(
Expand All @@ -21,7 +25,7 @@ def call

t1.nested do |t2|
t2.insert_separated_list(
["id"] + (object.attributes.keys.sort - ["id"])
[id] + (object.attributes.keys.sort - [id])
) do |t3, name|
t3.as_prefix_when_rendering_to_lines do |t4|
t4.add_text "#{name}: "
Expand Down
8 changes: 5 additions & 3 deletions lib/super_diff/active_record/monkey_patches.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,11 @@
class ActiveRecord::Base
# TODO: Remove this monkey patch if possible
def attributes_for_super_diff
(attributes.keys.sort - ["id"]).reduce({ id: id }) do |hash, key|
hash.merge(key.to_sym => attributes[key])
end
id_attr = self.class.primary_key

(attributes.keys.sort - [id_attr]).reduce(
{ id_attr.to_sym => id }
) { |hash, key| hash.merge(key.to_sym => attributes[key]) }
end
end
# rubocop:enable Style/BracesAroundHashParameters, Style/ClassAndModuleChildren
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,12 @@ def self.applies_to?(expected, actual)

protected

def id
expected.class.primary_key
end

def attribute_names
["id"] + (expected.attributes.keys.sort - ["id"])
[id] + (expected.attributes.keys.sort - [id])
end
end
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ def initialize(expected_output)
end

def removing_object_ids
first_replacing(/#<([\w:]+):0x[a-f0-9]+/, '#<\1')
first_replacing(/#<([\w_:]+):0x[a-f0-9]+/, '#<\1')
self
end

Expand Down
9 changes: 8 additions & 1 deletion spec/support/models/active_record/person.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ module Test
module Models
module ActiveRecord
class Person < ::ActiveRecord::Base
self.primary_key = "person_id"
end
end
end
Expand All @@ -13,7 +14,13 @@ class Person < ::ActiveRecord::Base
config.before do
ActiveRecord::Base
.connection
.create_table(:people, force: true) do |t|
.create_table(
:people,
id: false,
primary_key: "person_id",
force: true
) do |t|
t.primary_key :person_id, null: false
t.string :name, null: false
t.integer :age, null: false
end
Expand Down
10 changes: 5 additions & 5 deletions spec/support/shared_examples/active_record.rb
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@
proc do
line do
plain "Expected "
actual %|#<SuperDiff::Test::Models::ActiveRecord::Person id: nil, age: 31, name: "Elliot">|
actual %|#<SuperDiff::Test::Models::ActiveRecord::Person person_id: nil, age: 31, name: "Elliot">|
end

line do
Expand Down Expand Up @@ -244,7 +244,7 @@
proc do
line do
plain "Expected "
actual %|{ name: "Marty McFly", shipping_address: #<SuperDiff::Test::Models::ActiveRecord::Person id: nil, age: 31, name: "Elliot"> }|
actual %|{ name: "Marty McFly", shipping_address: #<SuperDiff::Test::Models::ActiveRecord::Person person_id: nil, age: 31, name: "Elliot"> }|
end

line do
Expand All @@ -265,7 +265,7 @@
expected_line %|- zip: "90382"|
expected_line "- }>"
actual_line "+ shipping_address: #<SuperDiff::Test::Models::ActiveRecord::Person {"
actual_line "+ id: nil,"
actual_line "+ person_id: nil,"
actual_line "+ age: 31,"
actual_line %|+ name: "Elliot"|
actual_line "+ }>"
Expand Down Expand Up @@ -390,7 +390,7 @@
proc do
line do
plain "Expected "
actual %|[#<SuperDiff::Test::Models::ActiveRecord::Query @results=#<ActiveRecord::Relation [#<SuperDiff::Test::Models::ActiveRecord::Person id: 1, age: 20, name: "Murphy">]>>]|
actual %|[#<SuperDiff::Test::Models::ActiveRecord::Query @results=#<ActiveRecord::Relation [#<SuperDiff::Test::Models::ActiveRecord::Person person_id: 1, age: 20, name: "Murphy">]>>]|
end

line do
Expand All @@ -404,7 +404,7 @@
plain_line " #<SuperDiff::Test::Models::ActiveRecord::Query {"
plain_line " @results=#<ActiveRecord::Relation ["
plain_line " #<SuperDiff::Test::Models::ActiveRecord::Person {"
plain_line " id: 1,"
plain_line " person_id: 1,"
# expected_line %|- age: 19,| # TODO
expected_line "- age: 19"
actual_line "+ age: 20,"
Expand Down
4 changes: 2 additions & 2 deletions spec/unit/active_record/object_inspection_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
as_lines: false
)
expect(string).to eq(
%(#<SuperDiff::Test::Models::ActiveRecord::Person id: nil, age: 31, name: "Elliot">)
%(#<SuperDiff::Test::Models::ActiveRecord::Person person_id: nil, age: 31, name: "Elliot">)
)
end
end
Expand Down Expand Up @@ -91,7 +91,7 @@
)

expect(string).to eq(
%(#<ActiveRecord::Relation [#<SuperDiff::Test::Models::ActiveRecord::Person id: 1, age: 19, name: "Marty">, #<SuperDiff::Test::Models::ActiveRecord::Person id: 2, age: 17, name: "Jennifer">]>)
%(#<ActiveRecord::Relation [#<SuperDiff::Test::Models::ActiveRecord::Person person_id: 1, age: 19, name: "Marty">, #<SuperDiff::Test::Models::ActiveRecord::Person id: 2, age: 17, name: "Jennifer">]>)
)
end
end
Expand Down

0 comments on commit cb9b75c

Please sign in to comment.