From 7ffc318dbab023725be3211ce309911ac04a3aed Mon Sep 17 00:00:00 2001
From: benk-gc <bkyriakou@gocardless.com>
Date: Mon, 1 Apr 2024 14:50:27 +0100
Subject: [PATCH] Support the use of primary keys other than "id" in
 ActiveRecord.

Previously the diffing code made an assumption that "id" would always be
the primary field. This is not always the case. Additionally, 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.
---
 .../inspection_tree_builders/active_record_model.rb  | 12 ++++++++++--
 lib/super_diff/active_record/monkey_patches.rb       |  8 +++++---
 .../operation_tree_builders/active_record_model.rb   |  6 +++++-
 .../matchers/produce_output_when_run_matcher.rb      |  2 +-
 spec/support/models/active_record/person.rb          |  9 ++++++++-
 spec/support/shared_examples/active_record.rb        | 10 +++++-----
 spec/unit/active_record/object_inspection_spec.rb    | 10 +++++-----
 7 files changed, 39 insertions(+), 18 deletions(-)

diff --git a/lib/super_diff/active_record/inspection_tree_builders/active_record_model.rb b/lib/super_diff/active_record/inspection_tree_builders/active_record_model.rb
index 25bf2a2c..84929abf 100644
--- a/lib/super_diff/active_record/inspection_tree_builders/active_record_model.rb
+++ b/lib/super_diff/active_record/inspection_tree_builders/active_record_model.rb
@@ -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(
@@ -21,13 +25,17 @@ 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}: "
                 end
 
-                t3.add_inspection_of object.read_attribute(name)
+                if name == id
+                  t3.add_inspection_of object.id
+                else
+                  t3.add_inspection_of object.read_attribute(name)
+                end
               end
             end
 
diff --git a/lib/super_diff/active_record/monkey_patches.rb b/lib/super_diff/active_record/monkey_patches.rb
index b2ebce7e..52ae6634 100644
--- a/lib/super_diff/active_record/monkey_patches.rb
+++ b/lib/super_diff/active_record/monkey_patches.rb
@@ -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
diff --git a/lib/super_diff/active_record/operation_tree_builders/active_record_model.rb b/lib/super_diff/active_record/operation_tree_builders/active_record_model.rb
index 72442b08..832462ea 100644
--- a/lib/super_diff/active_record/operation_tree_builders/active_record_model.rb
+++ b/lib/super_diff/active_record/operation_tree_builders/active_record_model.rb
@@ -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
diff --git a/spec/support/integration/matchers/produce_output_when_run_matcher.rb b/spec/support/integration/matchers/produce_output_when_run_matcher.rb
index 66859e40..43d3a9fd 100644
--- a/spec/support/integration/matchers/produce_output_when_run_matcher.rb
+++ b/spec/support/integration/matchers/produce_output_when_run_matcher.rb
@@ -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
 
diff --git a/spec/support/models/active_record/person.rb b/spec/support/models/active_record/person.rb
index bf8ec2c5..c86a31e7 100644
--- a/spec/support/models/active_record/person.rb
+++ b/spec/support/models/active_record/person.rb
@@ -3,6 +3,7 @@ module Test
     module Models
       module ActiveRecord
         class Person < ::ActiveRecord::Base
+          self.primary_key = "person_id"
         end
       end
     end
@@ -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
diff --git a/spec/support/shared_examples/active_record.rb b/spec/support/shared_examples/active_record.rb
index 076c0e88..4a172f4e 100644
--- a/spec/support/shared_examples/active_record.rb
+++ b/spec/support/shared_examples/active_record.rb
@@ -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
@@ -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
@@ -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 "+   }>"
@@ -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
@@ -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,"
diff --git a/spec/unit/active_record/object_inspection_spec.rb b/spec/unit/active_record/object_inspection_spec.rb
index fdd6a140..9fce79a9 100644
--- a/spec/unit/active_record/object_inspection_spec.rb
+++ b/spec/unit/active_record/object_inspection_spec.rb
@@ -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
@@ -42,7 +42,7 @@
               an_object_having_attributes(
                 type: :delete,
                 indentation_level: 2,
-                prefix: "id: ",
+                prefix: "person_id: ",
                 value: "nil",
                 add_comma: true
               ),
@@ -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 person_id: 2, age: 17, name: "Jennifer">]>)
           )
         end
       end
@@ -132,7 +132,7 @@
               an_object_having_attributes(
                 type: :delete,
                 indentation_level: 3,
-                prefix: "id: ",
+                prefix: "person_id: ",
                 value: "1",
                 add_comma: true
               ),
@@ -165,7 +165,7 @@
               an_object_having_attributes(
                 type: :delete,
                 indentation_level: 3,
-                prefix: "id: ",
+                prefix: "person_id: ",
                 value: "2",
                 add_comma: true
               ),