Fix model attribute accessors

CHANGELOG.md

@@ -6,10 +6,12 @@ Breaking changes:
 
 Features:
 
-- [#2021](https://github.com/rails-api/active_model_serializers/pull/2021) ActiveModelSerializers::Model#attributes. (@bf4)
+- [#2021](https://github.com/rails-api/active_model_serializers/pull/2021) ActiveModelSerializers::Model#attributes. Originally in [#1982](https://github.com/rails-api/active_model_serializers/pull/1982). (@bf4)
 
 Fixes:
 
+- [#2022](https://github.com/rails-api/active_model_serializers/pull/2022) Mutation of ActiveModelSerializers::Model now changes the attributes. Originally in [#1984](https://github.com/rails-api/active_model_serializers/pull/1984). (@bf4)
+
 Misc:
 
 - [#2021](https://github.com/rails-api/active_model_serializers/pull/2021) Make test attributes explicit. Tests have Model#associations. (@bf4)

docs/howto/serialize_poro.md

@@ -29,7 +29,7 @@ define and validate which methods ActiveModelSerializers expects to be implement
 
 An implementation of the complete spec is included either for use or as reference:
 [`ActiveModelSerializers::Model`](../../lib/active_model_serializers/model.rb).
-You can use in production code that will make your PORO a lot cleaner.  
+You can use in production code that will make your PORO a lot cleaner.
 
 The above code now becomes:
 

lib/active_model_serializers/model.rb

@@ -6,6 +6,17 @@ class Model
     include ActiveModel::Serializers::JSON
     include ActiveModel::Model
 
+    # Declare names of attributes to be included in +sttributes+ hash.
+    # Is only available as a class-method since the ActiveModel::Serialization mixin in Rails
+    # uses an +attribute_names+ local variable, which may conflict if we were to add instance methods here.
+    #
+    # @overload attribute_names
+    #   @return [Array<Symbol>]
+    class_attribute :attribute_names, instance_writer: false, instance_reader: false
+    # Initialize +attribute_names+ for all subclasses.  The array is usually
+    # mutated in the +attributes+ method, but can be set directly, as well.
+    self.attribute_names = []
+
     # Easily declare instance attributes with setters and getters for each.
     #
     # All attributes to initialize an instance must have setters.
@@ -25,12 +36,46 @@ class Model
     #   @param names [Array<String, Symbol>]
     #   @param name [String, Symbol]
     def self.attributes(*names)
+      self.attribute_names |= names.map(&:to_sym)
       # Silence redefinition of methods warnings
       ActiveModelSerializers.silence_warnings do
         attr_accessor(*names)
       end
     end
 
+    # Opt-in to breaking change
+    def self.derive_attributes_from_names_and_fix_accessors
+      unless included_modules.include?(DeriveAttributesFromNamesAndFixAccessors)
+        prepend(DeriveAttributesFromNamesAndFixAccessors)
+      end
+    end
+
+    module DeriveAttributesFromNamesAndFixAccessors
+      def self.included(base)
+        # NOTE that +id+ will always be in +attributes+.
+        base.attributes :id
+      end
+
+      # Override the initialize method so that attributes aren't processed.
+      #
+      # @param attributes [Hash]
+      def initialize(attributes = {})
+        @errors = ActiveModel::Errors.new(self)
+        super
+      end
+
+      # Override the +attributes+ method so that the hash is derived from +attribute_names+.
+      #
+      # The the fields in +attribute_names+ determines the returned hash.
+      # +attributes+ are returned frozen to prevent any expectations that mutation affects
+      # the actual values in the model.
+      def attributes
+        self.class.attribute_names.each_with_object({}) do |attribute_name, result|
+          result[attribute_name] = public_send(attribute_name).freeze
+        end.with_indifferent_access.freeze
+      end
+    end
+
     # Support for validation and other ActiveModel::Errors
     # @return [ActiveModel::Errors]
     attr_reader :errors

test/action_controller/adapter_selector_test.rb

@@ -3,6 +3,15 @@
 module ActionController
   module Serialization
     class AdapterSelectorTest < ActionController::TestCase
+      class Profile < Model
+        attributes :id, :name, :description
+        associations :comments
+      end
+      class ProfileSerializer < ActiveModel::Serializer
+        type 'profiles'
+        attributes :name, :description
+      end
+
       class AdapterSelectorTestController < ActionController::Base
         def render_using_default_adapter
           @profile = Profile.new(name: 'Name 1', description: 'Description 1', comments: 'Comments 1')

test/action_controller/namespace_lookup_test.rb

@@ -4,7 +4,7 @@ module ActionController
   module Serialization
     class NamespaceLookupTest < ActionController::TestCase
       class Book < ::Model
-        attributes :title, :body
+        attributes :id, :title, :body
         associations :writer, :chapters
       end
       class Chapter < ::Model
@@ -86,15 +86,15 @@ def explicit_namespace_as_string
               book = Book.new(title: 'New Post', body: 'Body')
 
               # because this is a string, ruby can't auto-lookup the constant, so otherwise
-              # the looku things we mean ::Api::V2
+              # the lookup thinks we mean ::Api::V2
               render json: book, namespace: 'ActionController::Serialization::NamespaceLookupTest::Api::V2'
             end
 
             def explicit_namespace_as_symbol
               book = Book.new(title: 'New Post', body: 'Body')
 
               # because this is a string, ruby can't auto-lookup the constant, so otherwise
-              # the looku things we mean ::Api::V2
+              # the lookup thinks we mean ::Api::V2
               render json: book, namespace: :'ActionController::Serialization::NamespaceLookupTest::Api::V2'
             end
 

test/active_model_serializers/model_test.rb

@@ -49,6 +49,33 @@ def test_attributes_can_be_read_for_serialization
       assert_equal 1, instance.read_attribute_for_serialization(:one)
     end
 
+    def test_attributes_can_be_read_for_serialization_with_attributes_accessors_fix
+      klass = Class.new(ActiveModelSerializers::Model) do
+        derive_attributes_from_names_and_fix_accessors
+        attributes :one, :two, :three
+      end
+      original_attributes = { one: 1, two: 2, three: 3 }
+      original_instance = klass.new(original_attributes)
+
+      # Initial value
+      instance = original_instance
+      expected_attributes = { one: 1, two: 2, three: 3 }.with_indifferent_access
+      assert_equal expected_attributes, instance.attributes
+      assert_equal 1, instance.one
+      assert_equal 1, instance.read_attribute_for_serialization(:one)
+
+      expected_attributes = { one: :not_one, two: 2, three: 3 }.with_indifferent_access
+      # Change via accessor
+      instance = original_instance.dup
+      instance.one = :not_one
+      assert_equal expected_attributes, instance.attributes
+      assert_equal :not_one, instance.one
+      assert_equal :not_one, instance.read_attribute_for_serialization(:one)
+
+      # Attributes frozen
+      assert instance.attributes.frozen?
+    end
+
     def test_id_attribute_can_be_read_for_serialization
       klass = Class.new(ActiveModelSerializers::Model) do
         attributes :id, :one, :two, :three
@@ -81,5 +108,35 @@ def test_id_attribute_can_be_read_for_serialization
     ensure
       self.class.send(:remove_const, :SomeTestModel)
     end
+
+    def test_id_attribute_can_be_read_for_serialization_with_attributes_accessors_fix
+      klass = Class.new(ActiveModelSerializers::Model) do
+        derive_attributes_from_names_and_fix_accessors
+        attributes :id, :one, :two, :three
+      end
+      self.class.const_set(:SomeTestModel, klass)
+      original_attributes = { id: :ego, one: 1, two: 2, three: 3 }
+      original_instance = klass.new(original_attributes)
+
+      # Initial value
+      instance = original_instance.dup
+      expected_attributes = { id: :ego, one: 1, two: 2, three: 3 }.with_indifferent_access
+      assert_equal expected_attributes, instance.attributes
+      assert_equal :ego, instance.id
+      assert_equal :ego, instance.read_attribute_for_serialization(:id)
+
+      expected_attributes = { id: :superego, one: 1, two: 2, three: 3 }.with_indifferent_access
+      # Change via accessor
+      instance = original_instance.dup
+      instance.id = :superego
+      assert_equal expected_attributes, instance.attributes
+      assert_equal :superego, instance.id
+      assert_equal :superego, instance.read_attribute_for_serialization(:id)
+
+      # Attributes frozen
+      assert instance.attributes.frozen?
+    ensure
+      self.class.send(:remove_const, :SomeTestModel)
+    end
   end
 end

test/adapter/json/has_many_test.rb

@@ -4,6 +4,10 @@ module ActiveModelSerializers
   module Adapter
     class Json
       class HasManyTestTest < ActiveSupport::TestCase
+        class ModelWithoutSerializer < ::Model
+          attributes :id, :name
+        end
+
         def setup
           ActionController::Base.cache_store.clear
           @author = Author.new(id: 1, name: 'Steve K.')
@@ -16,7 +20,7 @@ def setup
           @second_comment.post = @post
           @blog = Blog.new(id: 1, name: 'My Blog!!')
           @post.blog = @blog
-          @tag = Tag.new(id: 1, name: '#hash_tag')
+          @tag = ModelWithoutSerializer.new(id: 1, name: '#hash_tag')
           @post.tags = [@tag]
         end
 
@@ -30,7 +34,11 @@ def test_has_many
         end
 
         def test_has_many_with_no_serializer
-          serializer = PostWithTagsSerializer.new(@post)
+          post_serializer_class = Class.new(ActiveModel::Serializer) do
+            attributes :id
+            has_many :tags
+          end
+          serializer = post_serializer_class.new(@post)
           adapter = ActiveModelSerializers::Adapter::Json.new(serializer)
           assert_equal({
             id: 42,

test/adapter/json_api/has_many_test.rb

@@ -4,6 +4,10 @@ module ActiveModelSerializers
   module Adapter
     class JsonApi
       class HasManyTest < ActiveSupport::TestCase
+        class ModelWithoutSerializer < ::Model
+          attributes :id, :name
+        end
+
         def setup
           ActionController::Base.cache_store.clear
           @author = Author.new(id: 1, name: 'Steve K.')
@@ -26,7 +30,7 @@ def setup
           @blog.articles = [@post]
           @post.blog = @blog
           @post_without_comments.blog = nil
-          @tag = Tag.new(id: 1, name: '#hash_tag')
+          @tag = ModelWithoutSerializer.new(id: 1, name: '#hash_tag')
           @post.tags = [@tag]
           @serializer = PostSerializer.new(@post)
           @adapter = ActiveModelSerializers::Adapter::JsonApi.new(@serializer)
@@ -129,7 +133,11 @@ def test_include_type_for_association_when_different_than_name
         end
 
         def test_has_many_with_no_serializer
-          serializer = PostWithTagsSerializer.new(@post)
+          post_serializer_class = Class.new(ActiveModel::Serializer) do
+            attributes :id
+            has_many :tags
+          end
+          serializer = post_serializer_class.new(@post)
           adapter = ActiveModelSerializers::Adapter::JsonApi.new(serializer)
 
           assert_equal({

test/adapter/json_api/include_data_if_sideloaded_test.rb

@@ -14,11 +14,21 @@ def all
               [{ foo: 'bar' }]
             end
           end
+          class Tag < ::Model
+            attributes :id, :name
+          end
 
           class TagSerializer < ActiveModel::Serializer
+            type 'tags'
             attributes :id, :name
           end
 
+          class PostWithTagsSerializer < ActiveModel::Serializer
+            type 'posts'
+            attributes :id
+            has_many :tags
+          end
+
           class IncludeParamAuthorSerializer < ActiveModel::Serializer
             class_attribute :comment_loader