Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lib/active_resource.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
12 changes: 12 additions & 0 deletions lib/active_resource/attribute_set.rb
Original file line number Diff line number Diff line change
@@ -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
16 changes: 12 additions & 4 deletions lib/active_resource/base.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
# <tt>ActiveResource::schema=</tt>), the default automatic schema is
# generated from the current instance's attributes
Expand Down Expand Up @@ -1385,7 +1393,7 @@ def id

# Sets the <tt>\id</tt> 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
Expand Down Expand Up @@ -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
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This really sucks. I don't think it is worth a deprecation if we are going to start using send and instance_variable_set inside the framework code. # :nodoc: should be enough. I'll revert it

Copy link
Copy Markdown
Contributor Author

@seanpdoyle seanpdoyle Jan 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rafaelfranca that feedback is valid. Deprecating assignment is mainly for the sake of #410.

If #410 is not viable, this isn't worthwhile.

If #410 is viable, this might be worth reconsidering. Rather than send :instance_variable_set, would wrapping this call to #attributes= in a silence block be a good enough temporary solution so that the deprecation could be released ahead of #410 being released?

Suggested change
resource.send :instance_variable_set, "@attributes", @attributes
ActiveResource.deprecator.silence { resource.attributes = @attributes }

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

attributes= was never public, so I'm not sure why we need to deprecate it in order to add ActiveModel::Attributes. ActiveModel::Attributes don't define attributes=, only attributes.

Also, now attributes= is just defaulting to the method_missing behavior since it is protected. So I will deprecate it there.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

attributes= was never public,

You're right -- the attr_accessor :attributes was marked as # :nodoc:

https://github.com/rails/activeresource/blob/v6.1.4/lib/active_resource/base.rb#L1184

The idea to deprecate the accessor came from it being Ruby-level public. Since it's omitted from the public documentation, I defer to you on whether or not a deprecation cycle is necessary. Skipping the deprecation cycle would simplify things a lot.

resource.prefix_options = @prefix_options
end
end
Expand Down Expand Up @@ -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
Expand Down
34 changes: 34 additions & 0 deletions test/cases/base_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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