Deprecate using id for custom primary keys

During our work on composite keys in Rails it became clear that we were
conflating the meaning of `id`. In most cases applications use the `id`
column as the primary key. However Rails allows you to set a custom
primary key and use `id` to call that. For example if `Post` has a
primary key named `title` calling `Post.first.id` will return the title,
even if there is an `id` column on the posts table.

Essenstially the problem is that by treating `id` as special there is no
good API for accessing or setting an `id` attribute on a record with a
custom primary key.

This PR proposes removing the special meaning for `id`. If an
application is using a standard `id` column as the primary key, no
changes will be required. If an application does have a custom primary
key Rails will fire a deprecation warning and point them to use the
`primary_key_*` methods instead. This will also be useful for composite
primary keys becasue it's likely many applications will use keys like
`[:shop_id, :id]`. Without this change the only way to access the actual
id attribute would be `post.attributes["id"]` or
`post._read_attribute("id")`. With this change applications can access
the full key with `post.primary_key_value` or just the `id` attribute
with `post.id`.

activerecord/CHANGELOG.md

@@ -1,3 +1,22 @@
+*   Deprecate using `id` for primary key when an application has a custom primary key.
+
+    Previously the `id` method had a double meaning. In an application that uses `id` as the primary key this would return the `id` attribute. However, applications with custom primary keys would return the custom key value even if the table had an `id` attribute. This behavior is now deprecated.
+
+    Applications that use `id` as the primary key require no changes. Applications that do use custom primary keys will need to update their application to call the new method `primary_key_value`. Applications can opt into the new behavior by setting `config.active_record.use_id_as_attribute = true`.
+
+    Mapping of old methods to new methods:
+
+    * `id` => `primary_key_value`
+    * `id=(value)` => `primary_key_value=(value)`
+    * `id?` => `primary_key_value?`
+    * `id_was` => `primary_key_was`
+    * `id_in_database` => `primary_key_in_database`
+    * `id_before_type_cast` => `primary_key_before_type_cast`
+
+    This change will provide new public APIs for accessing custom primary keys which will restore the ability to query the `id` column if it exists and is not the primary key.
+
+    *Eileen M. Uchitelle*
+
 *   `after_commit` callbacks defined on models now execute in the correct order.
 
     ```ruby

activerecord/lib/active_record.rb

@@ -440,6 +440,13 @@ def self.suppress_multiple_database_warning=(value)
   singleton_class.attr_accessor :yaml_column_permitted_classes
   self.yaml_column_permitted_classes = [Symbol]
 
+  ##
+  # :singleton-method:
+  # Application configuration to always treat +id+ as an attribute rather than
+  # a primary key. When true, use +primary_key_value+ to get the primary key.
+  singleton_class.attr_accessor :use_id_as_attribute
+  self.use_id_as_attribute = false
+
   def self.marshalling_format_version
     Marshalling.format_version
   end

activerecord/lib/active_record/associations/has_many_through_association.rb

@@ -156,7 +156,7 @@ def delete_records(records, method)
 
           if source_reflection.options[:counter_cache] && method != :destroy
             counter = source_reflection.counter_cache_column
-            klass.decrement_counter counter, records.map(&:id)
+            klass.decrement_counter counter, records.map(&:primary_key_value)
           end
 
           if through_reflection.collection? && update_through_counter?(method)

activerecord/lib/active_record/attribute_methods.rb

@@ -409,7 +409,7 @@ def attributes_for_update(attribute_names)
       def attributes_for_create(attribute_names)
         attribute_names &= self.class.column_names
         attribute_names.delete_if do |name|
-          (pk_attribute?(name) && id.nil?) ||
+          (pk_attribute?(name) && primary_key_value.nil?) ||
             column_for_attribute(name).virtual?
         end
       end

activerecord/lib/active_record/attribute_methods/primary_key.rb

@@ -10,64 +10,214 @@ module PrimaryKey
       # Returns this record's primary key value wrapped in an array if one is
       # available.
       def to_key
-        key = id
+        key = primary_key_value
         [key] if key
       end
 
-      # Returns the primary key column's value.
+      # If +use_id_as_attribute+ is set, this method will return the
+      # +id+ attribute from the database. If it is not set, it returns
+      # the primary key column's value. The latter is deprecated.
       def id
-        return _read_attribute(@primary_key) unless @primary_key.is_a?(Array)
+        if ActiveRecord.use_id_as_attribute
+          _read_attribute("id")
+        else
+          if primary_key_is_not_id?
+            # deprecation
+            ActiveRecord.deprecator.warn(<<-MSG.squish)
+              You are using a custom primary key and calling `id` to access it.
+
+              Returning the primary key's value when calling `id` is deprecated. In Rails
+              7.2 this method will return the value from the database for the `id` attribute
+              if defined. To get the primary key's value, use `primary_key_value`.
+            MSG
+          end
 
-        @primary_key.map { |pk| _read_attribute(pk) }
+          primary_key_value
+        end
+      end
+
+      # Returns the primary key column's value.
+      def primary_key_value
+        if @primary_key.is_a?(Array)
+          @primary_key.map { |pk| _read_attribute(pk) }
+        else
+          _read_attribute(@primary_key)
+        end
       end
 
       def primary_key_values_present? # :nodoc:
-        return id.all? if self.class.composite_primary_key?
+        return primary_key_value.all? if self.class.composite_primary_key?
 
-        !!id
+        !!primary_key_value
       end
 
-      # Sets the primary key column's value.
+      # If +use_id_as_attribute+ is set, this method will set the
+      # +id+ attribute in the database. If it is not set, it sets
+      # the primary key column's value. The latter is deprecated.
       def id=(value)
+        if ActiveRecord.use_id_as_attribute
+          _write_attribute("id", value)
+        else
+          if primary_key_is_not_id?
+            ActiveRecord.deprecator.warn(<<-MSG.squish)
+              You are using a custom primary key and calling `id=` to set it.
+
+              Setting the primary key's value when calling `id=` is deprecated. In Rails
+              7.2 this method will set the value from the database for the `id` attribute
+              if defined. To set the primary key's value, use `primary_key_value=`.
+            MSG
+          end
+
+          self.primary_key_value = value
+        end
+      end
+
+      # Sets the primary key column's value.
+      def primary_key_value=(value)
         if self.class.composite_primary_key?
           @primary_key.zip(value) { |attr, value| _write_attribute(attr, value) }
         else
           _write_attribute(@primary_key, value)
         end
       end
 
-      # Queries the primary key column's value.
+      # If +use_id_as_attribute+ is set, this method will query the
+      # +id+ column's value. Otherwise this will query the primary key
+      # column's value. If you are using a custom primary key, use the
+      # +primary_key_value?+ method to avoid the deprecation warning.
       def id?
+        if ActiveRecord.use_id_as_attribute
+          query_attribute(ID_COLUMN)
+        else
+          if primary_key_is_not_id?
+            ActiveRecord.deprecator.warn(<<-MSG.squish)
+              Calling `id?` for a custom primary key is deprecated. In
+              7.2 this method will query the `id` column's value. To
+              query the primary key column's value use `primary_key_value?`
+            MSG
+          end
+
+          primary_key_value?
+        end
+      end
+
+      # Queries the primary key column's value.
+      def primary_key_value?
         query_attribute(@primary_key)
       end
 
-      # Returns the primary key column's value before type cast.
+      # If +use_id_as_attribute+ is set, this method will return the
+      # +id+ column's value before type casting. Otherwise this will
+      # return the primary key's value before type casting. If you are
+      # using a custom primary key, use the +primary_key_before_type_cast+
+      # method to avoid the deprecation warning.
       def id_before_type_cast
+        if ActiveRecord.use_id_as_attribute
+          attribute_before_type_cast(ID_COLUMN)
+        else
+          if primary_key_is_not_id?
+            ActiveRecord.deprecator.warn(<<-MSG.squish)
+              Calling `primary_key_before_type_cast` for a custom primary key is
+              deprecated. In 7.2 this method will return the value before type casting
+              for the `id` attribute. To get the value before type casting for the primary
+              key's use `primary_key_before_type_cast`.
+            MSG
+          end
+
+          primary_key_before_type_cast
+        end
+      end
+
+      # Returns the primary key column's value before type cast.
+      def primary_key_before_type_cast
         attribute_before_type_cast(@primary_key)
       end
 
-      # Returns the primary key column's previous value.
+      # If +use_id_as_attribute+ is set, this method will return the
+      # +id+ column's previous value. Otherwise this will return the
+      # primary key's previous value. If you are using a custom primary
+      # key, use the +primary_key_was+ method instead to avoid the
+      # deprecation warning.
       def id_was
+        if ActiveRecord.use_id_as_attribute
+          attribute_was(ID_COLUMN)
+        else
+          if primary_key_is_not_id?
+            ActiveRecord.deprecator.warn(<<-MSG.squish)
+              Calling `id_was` for a custom primary key is deprecated. In 7.2
+              this method will return the previous value for the `id` attribute. To
+              get the primary key's previous value use `primary_key_was`.
+            MSG
+          end
+
+          primary_key_was
+        end
+      end
+
+      # Returns the primary key column's previous value.
+      def primary_key_was
         attribute_was(@primary_key)
       end
 
-      # Returns the primary key column's value from the database.
+      # If +use_id_as_attribute+ is set, this method will return the
+      # +id+ attribute in the database. Otherwise this will return the
+      # primary key value in the database. If you are using a custom primary
+      # key, use the +primary_key_in_database+ method instead to avoid the
+      # deprecation warning.
       def id_in_database
+        if ActiveRecord.use_id_as_attribute
+          attribute_in_database(ID_COLUMN)
+        else
+          if primary_key_is_not_id?
+            ActiveRecord.deprecator.warn(<<-MSG.squish)
+              Calling `id_in_database` for a custom primary key is deprecated. In 7.2
+              this method will return the database value for the `id` attribute. To
+              get the primary key from the database use `primary_key_in_database`.
+            MSG
+          end
+
+          primary_key_in_database
+        end
+      end
+
+      # Returns the primary key column's value from the database.
+      def primary_key_in_database
         attribute_in_database(@primary_key)
       end
 
       def id_for_database # :nodoc:
+        if ActiveRecord.use_id_as_attribute
+          @attributes[ID_COLUMN].value_for_database
+        else
+          if primary_key_is_not_id?
+            ActiveRecord.deprecator.warn(<<-MSG.squish)
+              Calling `id_for_database` for a custom primary key is deprecated. In 7.2
+              this method will use the database value for the `id` attribute. To
+              use the primary key value for the database use `primary_key_for_database`.
+            MSG
+          end
+
+          primary_key_for_database
+        end
+      end
+
+      def primary_key_for_database # :nodoc:
         @attributes[@primary_key].value_for_database
       end
 
       private
+        def primary_key_is_not_id?
+          @primary_key != "id" #|| @primary_key != "ID"
+        end
+
         def attribute_method?(attr_name)
           attr_name == "id" || super
         end
 
         module ClassMethods
           ID_ATTRIBUTE_METHODS = %w(id id= id? id_before_type_cast id_was id_in_database id_for_database).to_set
           PRIMARY_KEY_NOT_SET = BasicObject.new
+          ID_COLUMN = "id"
 
           def instance_method_already_implemented?(method_name)
             super || primary_key && ID_ATTRIBUTE_METHODS.include?(method_name)

activerecord/lib/active_record/attribute_methods/read.rb

@@ -29,6 +29,7 @@ def read_attribute(attr_name, &block)
         name = attr_name.to_s
         name = self.class.attribute_aliases[name] || name
 
+        # TODO: need to fix this as part of work
         return @attributes.fetch_value(name, &block) unless name == "id" && @primary_key
 
         if self.class.composite_primary_key?

activerecord/lib/active_record/core.rb

@@ -555,15 +555,15 @@ def ==(comparison_object)
       super ||
         comparison_object.instance_of?(self.class) &&
         primary_key_values_present? &&
-        comparison_object.id == id &&
+        comparison_object.primary_key_value == primary_key_value &&
         !comparison_object.new_record?
     end
     alias :eql? :==
 
     # Delegates to id in order to allow two records of the same type and id to work with something like:
     #   [ Person.find(1), Person.find(2), Person.find(3) ] & [ Person.find(1), Person.find(4) ] # => [ Person.find(1) ]
     def hash
-      id = self.id
+      id = self.primary_key_value
 
       if primary_key_values_present? && !new_record?
         [self.class, id].hash

activerecord/lib/active_record/counter_cache.rb

@@ -65,7 +65,7 @@ def reset_counters(id, *counters, touch: nil)
           updates.merge!(touch_updates)
         end
 
-        unscoped.where(primary_key => object.id).update_all(updates) if updates.any?
+        unscoped.where(primary_key => object.primary_key_value).update_all(updates) if updates.any?
 
         true
       end
@@ -174,13 +174,13 @@ def counter_cache_column?(name) # :nodoc:
 
     private
       def _create_record(attribute_names = self.attribute_names)
-        id = super
+        primary_key_value = super
 
         each_counter_cached_associations do |association|
           association.increment_counters
         end
 
-        id
+        primary_key_value
       end
 
       def destroy_row

activerecord/lib/active_record/integration.rb

@@ -80,9 +80,9 @@ def cache_key
 
           if timestamp
             timestamp = timestamp.utc.to_fs(cache_timestamp_format)
-            "#{model_name.cache_key}/#{id}-#{timestamp}"
+            "#{model_name.cache_key}/#{primary_key_value}-#{timestamp}"
           else
-            "#{model_name.cache_key}/#{id}"
+            "#{model_name.cache_key}/#{primary_key_value}"
           end
         end
       end

activerecord/lib/active_record/nested_attributes.rb

@@ -431,7 +431,7 @@ def assign_nested_attributes_for_one_to_one_association(association_name, attrib
         existing_record = send(association_name)
 
         if (options[:update_only] || !attributes["id"].blank?) && existing_record &&
-            (options[:update_only] || existing_record.id.to_s == attributes["id"].to_s)
+            (options[:update_only] || existing_record.primary_key_value.to_s == attributes["id"].to_s)
           assign_to_or_mark_for_destruction(existing_record, attributes, options[:allow_destroy]) unless call_reject_if(association_name, attributes)
 
         elsif attributes["id"].present?
@@ -521,12 +521,12 @@ def assign_nested_attributes_for_collection_association(association_name, attrib
             unless reject_new_record?(association_name, attributes)
               association.reader.build(attributes.except(*UNASSIGNABLE_KEYS))
             end
-          elsif existing_record = existing_records.detect { |record| record.id.to_s == attributes["id"].to_s }
+          elsif existing_record = existing_records.detect { |record| record.primary_key_value.to_s == attributes["id"].to_s }
             unless call_reject_if(association_name, attributes)
               # Make sure we are operating on the actual object which is in the association's
               # proxy_target array (either by finding it, or adding it if not found)
               # Take into account that the proxy_target may have changed due to callbacks
-              target_record = association.target.detect { |record| record.id.to_s == attributes["id"].to_s }
+              target_record = association.target.detect { |record| record.primary_key_value.to_s == attributes["id"].to_s }
               if target_record
                 existing_record = target_record
               else