From b897313e9481656b8b18671768ef1f7e355eba40 Mon Sep 17 00:00:00 2001 From: Sean Doyle Date: Sun, 26 Oct 2025 09:41:23 -0400 Subject: [PATCH] Base: implement `read_attribute` and `write_attribute` Replace all access of `@attributes` and `attributes` key-value pairs with calls to `read_attribute` and `write_attribute`. The `read_attribute` and `write_attribute` implementations draw inspiration from [ActiveRecord::AttributeMethods::Read][] and [ActiveRecord::AttributeMethods::Write][], respectively. [rails/rails#53886][] proposes implementing each method at the Active Model layer. While that proposal is considered, this commit implements each method in terms of accessing the underlying `@attributes` hash instance. This change is also in support of a first-party integration with [ActiveModel::Attributes][] proposed in [#410][], and aims to be compatible with its attribute reading and writing interfaces. ```ruby person = Person.find(1) person.read_attribute("name") # => "Matz" person.name # => "Matz" person.write_attribute("name", "matz") person.name # => "matz" ``` [ActiveRecord::AttributeMethods::Read]: https://edgeapi.rubyonrails.org/classes/ActiveRecord/AttributeMethods/Read.html#method-i-read_attribute [ActiveRecord::AttributeMethods::Write]: https://edgeapi.rubyonrails.org/classes/ActiveRecord/AttributeMethods/Write.html#method-i-write_attribute [rails/rails#53886]: https://github.com/rails/rails/pull/53886 [ActiveModel::Attributes]: https://edgeapi.rubyonrails.org/classes/ActiveModel/Attributes.html [#410]: https://github.com/rails/activeresource/pull/410 --- lib/active_resource/associations.rb | 6 +- lib/active_resource/base.rb | 33 +++++++--- test/cases/attribute_methods_test.rb | 90 ++++++++++++++++++++++++++++ 3 files changed, 118 insertions(+), 11 deletions(-) create mode 100644 test/cases/attribute_methods_test.rb diff --git a/lib/active_resource/associations.rb b/lib/active_resource/associations.rb index 7d27bba2f8..4046f1c0eb 100644 --- a/lib/active_resource/associations.rb +++ b/lib/active_resource/associations.rb @@ -131,7 +131,7 @@ def defines_belongs_to_finder_method(reflection) if instance_variable_defined?(ivar_name) instance_variable_get(ivar_name) elsif attributes.include?(method_name) - attributes[method_name] + read_attribute(method_name) elsif association_id = send(reflection.foreign_key) instance_variable_set(ivar_name, reflection.klass.find(association_id)) end @@ -146,7 +146,7 @@ def defines_has_many_finder_method(reflection) if instance_variable_defined?(ivar_name) instance_variable_get(ivar_name) elsif attributes.include?(method_name) - attributes[method_name] + read_attribute(method_name) elsif !new_record? instance_variable_set(ivar_name, reflection.klass.find(:all, params: { "#{self.class.element_name}_id": self.id })) else @@ -164,7 +164,7 @@ def defines_has_one_finder_method(reflection) if instance_variable_defined?(ivar_name) instance_variable_get(ivar_name) elsif attributes.include?(method_name) - attributes[method_name] + read_attribute(method_name) elsif reflection.klass.respond_to?(:singleton_name) instance_variable_set(ivar_name, reflection.klass.find(params: { "#{self.class.element_name}_id": self.id })) else diff --git a/lib/active_resource/base.rb b/lib/active_resource/base.rb index edf92e5c25..ee5541435b 100644 --- a/lib/active_resource/base.rb +++ b/lib/active_resource/base.rb @@ -1380,12 +1380,12 @@ def persisted? # Gets the \id attribute of the resource. def id - attributes[self.class.primary_key] + read_attribute(self.class.primary_key) end # Sets the \id attribute of the resource. def id=(id) - attributes[self.class.primary_key] = id + write_attribute(self.class.primary_key, id) end # Test for equality. Resource are equal if and only if +other+ is the same object or @@ -1596,7 +1596,7 @@ def load(attributes, remove_root = false, persisted = false) attributes = Formats.remove_root(attributes) if remove_root attributes.each do |key, value| - @attributes[key.to_s] = + write_attribute(key.to_s, case value when Array resource = nil @@ -1614,6 +1614,7 @@ def load(attributes, remove_root = false, persisted = false) else value.duplicable? ? value.dup : value end + ) end self end @@ -1693,13 +1694,29 @@ def to_xml(options = {}) end def read_attribute_for_serialization(n) - if !attributes[n].nil? - attributes[n] + value = read_attribute(n) + + if !value.nil? + value elsif respond_to?(n) send(n) end end + def read_attribute(attr_name) + name = attr_name.to_s + + name = self.class.primary_key if name == "id" && self.class.primary_key + @attributes[name] + end + + def write_attribute(attr_name, value) + name = attr_name.to_s + + name = self.class.primary_key if name == "id" && self.class.primary_key + @attributes[name] = value + end + protected def connection(refresh = false) self.class.connection(refresh) @@ -1842,12 +1859,12 @@ def method_missing(method_symbol, *arguments) # :nodoc: if method_name =~ /(=|\?)$/ case $1 when "=" - attributes[$`] = arguments.first + write_attribute($`, arguments.first) when "?" - attributes[$`] + read_attribute($`) end else - return attributes[method_name] if attributes.include?(method_name) + return read_attribute(method_name) if attributes.include?(method_name) # not set right now but we know about it return nil if known_attributes.include?(method_name) super diff --git a/test/cases/attribute_methods_test.rb b/test/cases/attribute_methods_test.rb new file mode 100644 index 0000000000..8e4483903f --- /dev/null +++ b/test/cases/attribute_methods_test.rb @@ -0,0 +1,90 @@ +# frozen_string_literal: true + +require "abstract_unit" +require "fixtures/person" + +class AttributeMethodsTest < ActiveSupport::TestCase + setup :setup_response + + test "write_attribute string" do + matz = Person.find(1) + + assert_changes -> { matz.name }, to: "matz" do + matz.write_attribute("name", "matz") + end + end + + test "write_attribute symbol" do + matz = Person.find(1) + + assert_changes -> { matz.name }, to: "matz" do + matz.write_attribute(:name, "matz") + end + end + + test "write_attribute id" do + matz = Person.find(1) + + assert_changes -> { matz.id }, from: 1, to: "2" do + matz.write_attribute(:id, "2") + end + end + + test "write_attribute primary key" do + previous_primary_key = Person.primary_key + Person.primary_key = "pk" + matz = Person.find(1) + + assert_changes -> { matz.id }, from: 1, to: "2" do + matz.write_attribute(:id, "2") + end + assert_changes -> { matz.id }, from: "2", to: 1 do + matz.write_attribute("pk", 1) + end + assert_changes -> { matz.id }, from: 1, to: "2" do + matz.id = "2" + end + ensure + Person.primary_key = previous_primary_key + end + + test "write_attribute an unknown attribute" do + person = Person.new + + person.write_attribute("unknown", true) + + assert_predicate person, :unknown + end + + test "read_attribute" do + matz = Person.find(1) + + assert_equal "Matz", matz.read_attribute("name") + assert_equal "Matz", matz.read_attribute(:name) + end + + test "read_attribute id" do + matz = Person.find(1) + + assert_equal 1, matz.read_attribute("id") + assert_equal 1, matz.read_attribute(:id) + end + + test "read_attribute primary key" do + previous_primary_key = Person.primary_key + Person.primary_key = "pk" + matz = Person.find(1) + + assert_equal 1, matz.id + assert_equal 1, matz.read_attribute("pk") + assert_equal 1, matz.read_attribute(:pk) + ensure + Person.primary_key = previous_primary_key + end + + test "read_attribute unknown attribute" do + person = Person.new + + person.read_attribute("unknown") + end +end