Merge pull request #463 from seanpdoyle/deprecate-attributes-write

Author: guilleiguaran

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

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

lib/active_resource/base.rb

@@ -1291,9 +1291,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
@@ -1400,7 +1408,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
@@ -1454,7 +1462,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
@@ -1857,7 +1865,7 @@ def method_missing(method_symbol, *arguments) # :nodoc:
         if method_name =~ /(=|\?)$/
           case $1
           when "="
-            attributes[$`] = arguments.first
+            @attributes[$`] = arguments.first
           when "?"
             attributes[$`]
           end

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