Merge 0-10-stable into master (to fix breaking change).

CHANGELOG.md

@@ -6,16 +6,16 @@ Breaking changes:
 
 Features:
 
-- [#1982](https://github.com/rails-api/active_model_serializers/pull/1982) Add ActiveModelSerializers::Model.attributes to configure PORO 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:
 
-- [#1984](https://github.com/rails-api/active_model_serializers/pull/1984) Mutation of ActiveModelSerializers::Model now changes the attributes. (@bf4)
+- [#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)
 - [#1981](https://github.com/rails-api/active_model_serializers/pull/1981) Fix relationship link documentation. (@groyoh)
-- [#1984](https://github.com/rails-api/active_model_serializers/pull/1984) Make test attributes explicit. Test models have 'associations' support. (@bf4)
 
 ### [v0.10.4 (2017-01-06)](https://github.com/rails-api/active_model_serializers/compare/v0.10.3...v0.10.4)
 

docs/howto/serialize_poro.md

@@ -2,13 +2,16 @@
 
 # How to serialize a Plain-Old Ruby Object (PORO)
 
-When you are first getting started with ActiveModelSerializers, it may seem only `ActiveRecord::Base` objects can be serializable, but pretty much any object can be serializable with ActiveModelSerializers.  Here is an example of a PORO that is serializable:
+When you are first getting started with ActiveModelSerializers, it may seem only `ActiveRecord::Base` objects can be serializable,
+but pretty much any object can be serializable with ActiveModelSerializers.
+Here is an example of a PORO that is serializable in most situations:
+
 ```ruby
 # my_model.rb
 class MyModel
   alias :read_attribute_for_serialization :send
   attr_accessor :id, :name, :level
-  
+
   def initialize(attributes)
     @id = attributes[:id]
     @name = attributes[:name]
@@ -21,12 +24,22 @@ class MyModel
 end
 ```
 
-Fortunately, ActiveModelSerializers provides a [`ActiveModelSerializers::Model`](https://github.com/rails-api/active_model_serializers/blob/master/lib/active_model_serializers/model.rb) which you can use in production code that will make your PORO a lot cleaner.  The above code now becomes:
+The [ActiveModel::Serializer::Lint::Tests](../../lib/active_model/serializer/lint.rb)
+define and validate which methods ActiveModelSerializers expects to be implemented.
+
+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.
+
+The above code now becomes:
+
 ```ruby
 # my_model.rb
 class MyModel < ActiveModelSerializers::Model
   attributes :id, :name, :level
 end
 ```
 
-The default serializer would be `MyModelSerializer`.
+The default serializer would be `MyModelSerializer`.
+
+For more information, see [README: What does a 'serializable resource' look like?](../../README.md#what-does-a-serializable-resource-look-like).

lib/active_model_serializers/model.rb

@@ -1,16 +1,40 @@
-# ActiveModelSerializers::Model is a convenient
-# serializable class to inherit from when making
-# serializable non-activerecord objects.
+# ActiveModelSerializers::Model is a convenient superclass for making your models
+# from Plain-Old Ruby Objects (PORO). It also serves as a reference implementation
+# that satisfies ActiveModel::Serializer::Lint::Tests.
 module ActiveModelSerializers
   class Model
     include ActiveModel::Serializers::JSON
     include ActiveModel::Model
 
-    class_attribute :attribute_names
+    # 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.
+    # However, the hash turned by +attributes+ instance method will ALWAYS
+    # be the value of the initial attributes, regardless of what accessors are defined.
+    # The only way to change the change the attributes after initialization is
+    # to mutate the +attributes+ directly.
+    # Accessor methods do NOT mutate the attributes.  (This is a bug).
+    #
+    # @note For now, the Model only supports the notion of 'attributes'.
+    #   In the tests, there is a special Model that also supports 'associations'. This is
+    #   important so that we can add accessors for values that should not appear in the
+    #   attributes hash when modeling associations. It is not yet clear if it
+    #   makes sense for a PORO to have associations outside of the tests.
+    #
+    # @overload attributes(names)
+    #   @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
@@ -19,44 +43,97 @@ def self.attributes(*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
-    # NOTE that +updated_at+ isn't included in +attribute_names+,
-    # which means it won't show up in +attributes+ unless a subclass has
-    # either <tt>attributes :updated_at</tt> which will redefine the methods
-    # or <tt>attribute_names << :updated_at</tt>.
+
+    # (see #updated_at)
     attr_writer :updated_at
-    # NOTE that +id+ will always be in +attributes+.
-    attributes :id
 
+    # The only way to change the attributes of an instance is to directly mutate the attributes.
+    # @example
+    #
+    #   model.attributes[:foo] = :bar
+    # @return [Hash]
+    attr_reader :attributes
+
+    # @param attributes [Hash]
     def initialize(attributes = {})
+      attributes ||= {} # protect against nil
+      @attributes = attributes.symbolize_keys.with_indifferent_access
       @errors = ActiveModel::Errors.new(self)
       super
     end
 
-    # 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
-      attribute_names.each_with_object({}) do |attribute_name, result|
-        result[attribute_name] = public_send(attribute_name).freeze
-      end.with_indifferent_access.freeze
+    # Defaults to the downcased model name.
+    # This probably isn't a good default, since it's not a unique instance identifier,
+    # but that's what is currently implemented \_('-')_/.
+    #
+    # @note Though +id+ is defined, it will only show up
+    #   in +attributes+ when it is passed in to the initializer or added to +attributes+,
+    #   such as <tt>attributes[:id] = 5</tt>.
+    # @return [String, Numeric, Symbol]
+    def id
+      attributes.fetch(:id) do
+        defined?(@id) ? @id : self.class.model_name.name && self.class.model_name.name.downcase
+      end
+    end
+
+    # When not set, defaults to the time the file was modified.
+    #
+    # @note Though +updated_at+ and +updated_at=+ are defined, it will only show up
+    #   in +attributes+ when it is passed in to the initializer or added to +attributes+,
+    #   such as <tt>attributes[:updated_at] = Time.current</tt>.
+    # @return [String, Numeric, Time]
+    def updated_at
+      attributes.fetch(:updated_at) do
+        defined?(@updated_at) ? @updated_at : File.mtime(__FILE__)
+      end
     end
 
     # To customize model behavior, this method must be redefined. However,
     # there are other ways of setting the +cache_key+ a serializer uses.
+    # @return [String]
     def cache_key
       ActiveSupport::Cache.expand_cache_key([
         self.class.model_name.name.downcase,
         "#{id}-#{updated_at.strftime('%Y%m%d%H%M%S%9N')}"
       ].compact)
     end
 
-    # When no set, defaults to the time the file was modified.
-    # See NOTE by attr_writer :updated_at
-    def updated_at
-      defined?(@updated_at) ? @updated_at : File.mtime(__FILE__)
-    end
-
     # The following methods are needed to be minimally implemented for ActiveModel::Errors
     # :nocov:
     def self.human_attribute_name(attr, _options = {})

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

@@ -24,21 +24,56 @@ def test_attributes_can_be_read_for_serialization
         attributes :one, :two, :three
       end
       original_attributes = { one: 1, two: 2, three: 3 }
-      instance = klass.new(original_attributes)
+      original_instance = klass.new(original_attributes)
 
       # Initial value
-      expected_attributes = { id: nil, one: 1, two: 2, three: 3 }.with_indifferent_access
+      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)
 
-      # Change via accessor
+      # FIXME: Change via accessor has no effect on attributes.
+      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)
+
+      # FIXME: Change via mutating attributes
+      instance = original_instance.dup
+      instance.attributes[:one] = :not_one
+      expected_attributes = { one: :not_one, 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)
+    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 = { id: nil, one: :not_one, two: 2, three: 3 }.with_indifferent_access
+      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
@@ -47,21 +82,59 @@ def test_id_attribute_can_be_read_for_serialization
       end
       self.class.const_set(:SomeTestModel, klass)
       original_attributes = { id: :ego, one: 1, two: 2, three: 3 }
-      instance = klass.new(original_attributes)
+      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 1, instance.one
-      assert_equal 1, instance.read_attribute_for_serialization(:one)
+      assert_equal :ego, instance.id
+      assert_equal :ego, instance.read_attribute_for_serialization(:id)
 
-      # Change via accessor
+      # FIXME: Change via accessor has no effect on attributes.
+      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)
 
+      # FIXME: Change via mutating attributes
+      instance = original_instance.dup
+      instance.attributes[:id] = :superego
       expected_attributes = { id: :superego, 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)
+    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