Skip to content

Commit

Permalink
Deprecate read_attribute(:id) returning the primary key
Browse files Browse the repository at this point in the history
This commit deprecates `read_attribute(:id)` returning the primary key
if the model's primary key is not the id column. Starting in Rails 7.2,
`read_attribute(:id)` will always return the value of the id column.

This commit also changes `read_attribute(:id)` for composite primary
key models to return the value of the id column, not the composite
primary key.
  • Loading branch information
adrianna-chang-shopify committed Aug 23, 2023
1 parent dd6d931 commit ac83311
Show file tree
Hide file tree
Showing 4 changed files with 35 additions and 15 deletions.
8 changes: 8 additions & 0 deletions activerecord/CHANGELOG.md
@@ -1,3 +1,11 @@
* Deprecate `read_attribute(:id)` returning the primary key if the primary key is not `:id`.

Starting in Rails 7.2, `read_attribute(:id)` will return the value of the id column, regardless of the model's
primary key. To retrieve the value of the primary key, use `#id` instead. `read_attribute(:id)` for composite
primary key models will now return the value of the id column.

*Adrianna Chang*

* Fix `change_table` setting datetime precision for 6.1 Migrations

*Hartley McGuire*
Expand Down
Expand Up @@ -333,11 +333,7 @@ def merge_target_lists(persisted, memory)
if mem_record = memory.delete(record)

((record.attribute_names & mem_record.attribute_names) - mem_record.changed_attribute_names_to_save - mem_record.class._attr_readonly).each do |name|
if name == "id" && mem_record.class.composite_primary_key?
mem_record.class.primary_key.zip(record[name]) { |attr, value| mem_record._write_attribute(attr, value) }
else
mem_record._write_attribute(name, record[name])
end
mem_record._write_attribute(name, record[name])
end

mem_record
Expand Down
8 changes: 7 additions & 1 deletion activerecord/lib/active_record/attribute_methods/read.rb
Expand Up @@ -33,8 +33,14 @@ def read_attribute(attr_name, &block)
return @attributes.fetch_value(name, &block) unless name == "id" && @primary_key

if self.class.composite_primary_key?
@primary_key.map { |col| @attributes.fetch_value(col, &block) }
@attributes.fetch_value("id", &block)
else
if @primary_key != "id"
ActiveRecord.deprecator.warn(<<-MSG.squish)
Using read_attribute(:id) to read the primary key value is deprecated.
Use #id instead.
MSG
end
@attributes.fetch_value(@primary_key, &block)
end
end
Expand Down
28 changes: 19 additions & 9 deletions activerecord/test/cases/primary_keys_test.rb
Expand Up @@ -36,22 +36,32 @@ def test_to_key_with_composite_primary_key
assert_equal [1, 2], order.to_key
end

def test_read_attribute_id
topic = Topic.find(1)
id = assert_not_deprecated(ActiveRecord.deprecator) do
topic.read_attribute(:id)
end

assert_equal 1, id
end

def test_read_attribute_with_custom_primary_key
keyboard = Keyboard.create!
assert_equal keyboard.key_number, keyboard.read_attribute(:id)
msg = "Using read_attribute(:id) to read the primary key value is deprecated. Use #id instead."
id = assert_deprecated(msg, ActiveRecord.deprecator) do
keyboard.read_attribute(:id)
end

assert_equal keyboard.key_number, id
end

def test_read_attribute_with_composite_primary_key
book = Cpk::Book.new(id: [1, 2])
assert_equal [1, 2], book.read_attribute(:id)
end

def test_read_attribute_with_composite_primary_key_and_column_named_id
order = Cpk::Order.new
order.id = [1, 2]
id = assert_not_deprecated(ActiveRecord.deprecator) do
book.read_attribute(:id)
end

assert_equal [1, 2], order.read_attribute(:id)
assert_equal 2, order.attributes["id"]
assert_equal 2, id
end

def test_to_key_with_primary_key_after_destroy
Expand Down

0 comments on commit ac83311

Please sign in to comment.