From 55c71552dc3a9e5b76d3e14b908115e2d1bba65d Mon Sep 17 00:00:00 2001 From: Sean Doyle Date: Sun, 9 Nov 2025 15:15:04 -0500 Subject: [PATCH] Deprecate writing to `Base#attributes` The Base class defines an `attr_accessor :attributes` that is marked with `# :nodoc:`. Technically, this means that any interaction with `Base#attributes` or `Base#attributes=` is not part of the public interface, and is free to be changed or removed without breaking the public API. However, given the project's age and the long-term period of time with minimal changes to the public interface, this PR proposes that there be a public deprecation of **writing** to the Hash instance returned by the `Base#attributes` method. The migration to `ActiveModel::Attributes` proposed in [#410][] will involve changes to the `Base#attributes` method (due to [ActiveModel::Attributes#attributes][]). Reading from the value returned by `ActiveModel::Attributes#attributes` will remain the same, but writing to that value will have no affect since the value is a Hash copy created by [ActiveModel::AttributeSet#to_hash][], and not the Hash instance used internally. Similarly, `ActiveModel::Attributes` does not expose a corresponding `#attributes=` method. By deprecating `#attributes=`, changes made that incorporate `ActiveModel::Attributes` will not need to add an otherwise unnecessary `#attributes=` implementation. Once deprecated and part of a release cycle, the `ActiveResource::AttributeSet` class can be removed, and the Active Model migration's backwards compatibility burden can be reduced. [#410]: https://github.com/rails/activeresource/pull/410 [ActiveModel::Attributes#attributes]: https://edgeapi.rubyonrails.org/classes/ActiveModel/Attributes.html#method-i-attributes [ActiveModel::AttributeSet#to_hash]: https://github.com/rails/rails/blob/v8.1.1/activemodel/lib/active_model/attribute_set.rb#L36-L39 --- lib/active_resource.rb | 1 + lib/active_resource/attribute_set.rb | 12 ++++++++++ lib/active_resource/base.rb | 16 +++++++++---- test/cases/base_test.rb | 34 ++++++++++++++++++++++++++++ 4 files changed, 59 insertions(+), 4 deletions(-) create mode 100644 lib/active_resource/attribute_set.rb diff --git a/lib/active_resource.rb b/lib/active_resource.rb index 4379acd372..42b074aa53 100644 --- a/lib/active_resource.rb +++ b/lib/active_resource.rb @@ -35,6 +35,7 @@ module ActiveResource URI_PARSER = defined?(URI::RFC2396_PARSER) ? URI::RFC2396_PARSER : URI::RFC2396_Parser.new + autoload :AttributeSet autoload :Base autoload :Callbacks autoload :Coder diff --git a/lib/active_resource/attribute_set.rb b/lib/active_resource/attribute_set.rb new file mode 100644 index 0000000000..2ce7318075 --- /dev/null +++ b/lib/active_resource/attribute_set.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true + +module ActiveResource + class AttributeSet < DelegateClass(Hash) # :nodoc: + MESSAGE = "Writing to the attributes hash is deprecated. Set attributes directly on the instance instead." + + deprecate(**[ :[]=, :store, :update, :merge! ].index_with(MESSAGE), + deprecator: ActiveResource.deprecator) + + delegate :is_a?, to: :__getobj__ + end +end diff --git a/lib/active_resource/base.rb b/lib/active_resource/base.rb index edf92e5c25..291a3ddad8 100644 --- a/lib/active_resource/base.rb +++ b/lib/active_resource/base.rb @@ -1276,9 +1276,17 @@ def split_options(options = {}) end end - attr_accessor :attributes # :nodoc: attr_accessor :prefix_options # :nodoc: + def attributes=(value) # :nodoc: + ActiveResource.deprecator.warn("#attributes= is deprecated. Call #load on the instance instead.") + @attributes = value + end + + def attributes # :nodoc: + AttributeSet.new(@attributes) + end + # If no schema has been defined for the class (see # ActiveResource::schema=), the default automatic schema is # generated from the current instance's attributes @@ -1385,7 +1393,7 @@ def id # Sets the \id attribute of the resource. def id=(id) - attributes[self.class.primary_key] = id + @attributes[self.class.primary_key] = id end # Test for equality. Resource are equal if and only if +other+ is the same object or @@ -1439,7 +1447,7 @@ def hash # next_invoice.customer # => That Company def dup self.class.new.tap do |resource| - resource.attributes = @attributes + resource.send :instance_variable_set, "@attributes", @attributes resource.prefix_options = @prefix_options end end @@ -1842,7 +1850,7 @@ def method_missing(method_symbol, *arguments) # :nodoc: if method_name =~ /(=|\?)$/ case $1 when "=" - attributes[$`] = arguments.first + @attributes[$`] = arguments.first when "?" attributes[$`] end diff --git a/test/cases/base_test.rb b/test/cases/base_test.rb index 462920ac92..4d7dc579b1 100644 --- a/test/cases/base_test.rb +++ b/test/cases/base_test.rb @@ -1743,4 +1743,38 @@ def test_paths_without_format ensure ActiveResource::Base.include_format_in_path = true end + + def test_deprecate_attributes_write + person = Person.find(1) + + assert_deprecated("#attributes= is deprecated. Call #load on the instance instead.", ActiveResource.deprecator) do + person.attributes = { "name" => "changed" } + end + + assert_equal "changed", person.name + end + + def test_deprecate_attributes_store + [ :[]=, :store ].each do |store| + person = Person.find(1) + + assert_deprecated("Writing to the attributes hash is deprecated. Set attributes directly on the instance instead.", ActiveResource.deprecator) do + person.attributes.send(store, "name", "changed") + end + + assert_equal "changed", person.name + end + end + + def test_deprecate_attributes_update + [ :update, :merge! ].each do |update| + person = Person.find(1) + + assert_deprecated("Writing to the attributes hash is deprecated. Set attributes directly on the instance instead.", ActiveResource.deprecator) do + person.attributes.send(update, "name" => "changed") + end + + assert_equal "changed", person.name + end + end end