Avoid overwriting the methods of `AttributeMethods::PrimaryKey`

Currently the methods of `AttributeMethods::PrimaryKey` are overwritten
by `define_attribute_methods`. It will be broken if a table that
customized primary key has non primary key id column.
It should not be overwritten if a table has any primary key.

Fixes #29350.

4 files changed, 24 insertions, 19 deletions

diff --git a/activerecord/lib/active_record/attribute_methods/primary_key.rb b/activerecord/lib/active_record/attribute_methods/primary_key.rb
index 2f32caa257..b5fd0cb370 100644
--- a/activerecord/lib/active_record/attribute_methods/primary_key.rb
+++ b/activerecord/lib/active_record/attribute_methods/primary_key.rb
@@ -57,16 +57,12 @@ module ActiveRecord
         end
 
         module ClassMethods
-          def define_method_attribute(attr_name)
-            super
+          ID_ATTRIBUTE_METHODS = %w(id id= id? id_before_type_cast id_was id_in_database).to_set
 
-            if attr_name == primary_key && attr_name != "id"
-              generated_attribute_methods.send(:alias_method, :id, primary_key)
-            end
+          def instance_method_already_implemented?(method_name)
+            super || primary_key && ID_ATTRIBUTE_METHODS.include?(method_name)
           end
 
-          ID_ATTRIBUTE_METHODS = %w(id id= id? id_before_type_cast id_was id_in_database).to_set
-
           def dangerous_attribute_method?(method_name)
             super && !ID_ATTRIBUTE_METHODS.include?(method_name)
           end
diff --git a/activerecord/test/cases/primary_keys_test.rb b/activerecord/test/cases/primary_keys_test.rb
index 5ded619716..200d9e6434 100644
--- a/activerecord/test/cases/primary_keys_test.rb
+++ b/activerecord/test/cases/primary_keys_test.rb
@@ -46,7 +46,7 @@ class PrimaryKeysTest < ActiveRecord::TestCase
     topic = Topic.new
     topic.title = "New Topic"
     assert_nil topic.id
-    assert_nothing_raised { topic.save! }
+    topic.save!
     id = topic.id
 
     topicReloaded = Topic.find(id)
@@ -56,23 +56,30 @@ class PrimaryKeysTest < ActiveRecord::TestCase
   def test_customized_primary_key_auto_assigns_on_save
     Keyboard.delete_all
     keyboard = Keyboard.new(name: "HHKB")
-    assert_nothing_raised { keyboard.save! }
+    keyboard.save!
     assert_equal keyboard.id, Keyboard.find_by_name("HHKB").id
   end
 
   def test_customized_primary_key_can_be_get_before_saving
     keyboard = Keyboard.new
     assert_nil keyboard.id
-    assert_nothing_raised { assert_nil keyboard.key_number }
+    assert_nil keyboard.key_number
   end
 
   def test_customized_string_primary_key_settable_before_save
     subscriber = Subscriber.new
-    assert_nothing_raised { subscriber.id = "webster123" }
+    subscriber.id = "webster123"
     assert_equal "webster123", subscriber.id
     assert_equal "webster123", subscriber.nick
   end
 
+  def test_update_with_non_primary_key_id_column
+    subscriber = Subscriber.first
+    subscriber.update(update_count: 1)
+    subscriber.reload
+    assert_equal 1, subscriber.update_count
+  end
+
   def test_string_key
     subscriber = Subscriber.find(subscribers(:first).nick)
     assert_equal(subscribers(:first).name, subscriber.name)
@@ -83,7 +90,7 @@ class PrimaryKeysTest < ActiveRecord::TestCase
     subscriber.id = "jdoe"
     assert_equal("jdoe", subscriber.id)
     subscriber.name = "John Doe"
-    assert_nothing_raised { subscriber.save! }
+    subscriber.save!
     assert_equal("jdoe", subscriber.id)
 
     subscriberReloaded = Subscriber.find("jdoe")
diff --git a/activerecord/test/cases/schema_dumper_test.rb b/activerecord/test/cases/schema_dumper_test.rb
index 417c6f4832..b5386ba801 100644
--- a/activerecord/test/cases/schema_dumper_test.rb
+++ b/activerecord/test/cases/schema_dumper_test.rb
@@ -320,7 +320,7 @@ class SchemaDumperTest < ActiveRecord::TestCase
 
   def test_schema_dump_keeps_id_false_when_id_is_false_and_unique_not_null_column_added
     output = standard_dump
-    assert_match %r{create_table "subscribers", id: false}, output
+    assert_match %r{create_table "string_key_objects", id: false}, output
   end
 
   if ActiveRecord::Base.connection.supports_foreign_keys?
diff --git a/activerecord/test/schema/schema.rb b/activerecord/test/schema/schema.rb
index 50f1d9bfe7..8863736943 100644
--- a/activerecord/test/schema/schema.rb
+++ b/activerecord/test/schema/schema.rb
@@ -807,16 +807,18 @@ ActiveRecord::Schema.define do
     t.string :sponsorable_type
   end
 
-  create_table :string_key_objects, id: false, primary_key: :id, force: true do |t|
-    t.string     :id
-    t.string     :name
-    t.integer    :lock_version, null: false, default: 0
+  create_table :string_key_objects, id: false, force: true do |t|
+    t.string :id, null: false
+    t.string :name
+    t.integer :lock_version, null: false, default: 0
+    t.index :id, unique: true
   end
 
-  create_table :subscribers, force: true, id: false do |t|
+  create_table :subscribers, force: true do |t|
     t.string :nick, null: false
     t.string :name
-    t.column :books_count, :integer, null: false, default: 0
+    t.integer :books_count, null: false, default: 0
+    t.integer :update_count, null: false, default: 0
     t.index :nick, unique: true
   end