WIP: Restore legacy ams::model behavior

lib/active_model/serializer/null.rb

@@ -1,6 +1,7 @@
 module ActiveModel
   class Serializer
     class Null < Serializer
+      # :nocov:
       def attributes(*)
         {}
       end
@@ -12,6 +13,7 @@ def associations(*)
       def serializable_hash(*)
         {}
       end
+      # :nocov:
     end
   end
 end

lib/active_model_serializers/model.rb

@@ -1,33 +1,78 @@
-# ActiveModelSerializers::Model is a convenient
-# serializable class to inherit from when making
-# serializable non-activerecord objects.
+# ActiveModelSerializers::Model is a convenient superclass when making serializable non-activerecord objects.
+# It also serves as documentation of an implementation that satisfies ActiveModel::Serializer::Lint::Tests.
 module ActiveModelSerializers
   class Model
     include ActiveModel::Serializers::JSON
     include ActiveModel::Model
 
-    class_attribute :attribute_names
+    # @method :attributes
+    #   - unset attributes will appear in nil values in the attributes hash
+    #   - instance attributes can ONLY be changed by accessor methods.
+    #   - the return value of the attributes method is now frozen. since it is always derived, trying to mutate it is strongly discouraged :)
+    #
+    # @method :attributes_are_always_the_initialization_data
+    #
+    # When true, the deprecate attribute behavior is mixed in.
+    # Defaults to +true+, in order to avoid breaking changes with older versions of this class which lacked defined attributes.
+    #
+    # However, new apps probably want to change the default to false, and old apps want to create a special new model subclass where it is false for use in new models.
+    #
+    # Specifically, the deprecated behavior this re-adds is:
+    #   - the attributes hash returned by `#attributes` will always be the value of the attributes passed in at initialization,
+    #     regardless of what attr_* methods were defined.
+    #   - unset attributes, rather than being nil, will not be included in the attributes hash, even when an `attr_accessor` exists.
+    #   - the only way to change the change the attributes after initialization is to mutate the `attributes` directly.  Accessor methods
+    #     will NOT mutate the attributes.  (This is the bug that led to the change).
+    #
+    # For now, the Model only supports the notion of 'attributes'. In the tests, we do support 'associations' on the PORO,
+    # in order to adds accessors for values that should not appear in the attributes hash, as we model associations.
+    # However, it is not yet clear if it makes sense for a PORO to have associations outside of the tests.
+    class_attribute :attributes_are_always_the_initialization_data, instance_writer: false, instance_reader: false
+    self.attributes_are_always_the_initialization_data = true
+
+    def self.inherited(subclass)
+      if subclass.attributes_are_always_the_initialization_data
+        unless subclass.included_modules.include?(AttributesAreAlwaysTheInitializationData)
+          subclass.prepend(AttributesAreAlwaysTheInitializationData)
+
+          deprecated_method = :attributes_are_always_the_initialization_data
+          target = is_a?(Module) ? "#{self}." : "#{self.class}#"
+          msg = ["NOTE: #{target}#{deprecated_method} is deprecated with no replacement",
+                 "\n#{target}#{deprecated_method} called from #{ActiveModelSerializers.location_of_caller.join(':')}"]
+          warn "#{msg.join}."
+        end
+      end
+      super
+    end
+
+    # @method :attribute_names
+    # 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.
+    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 = []
 
     def self.attributes(*names)
       self.attribute_names |= names.map(&:to_sym)
-      # Silence redefinition of methods warnings
+      # Silence redefinition of methods warnings, since it is expected.
       ActiveModelSerializers.silence_warnings do
         attr_accessor(*names)
       end
     end
 
-    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>.
     attr_writer :updated_at
+
     # NOTE that +id+ will always be in +attributes+.
     attributes :id
 
+    # Support for validation and other ActiveModel::Errors
+    attr_reader :errors
+
     def initialize(attributes = {})
       @errors = ActiveModel::Errors.new(self)
       super
@@ -37,7 +82,7 @@ def initialize(attributes = {})
     # +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|
+      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
@@ -51,7 +96,7 @@ def cache_key
       ].compact)
     end
 
-    # When no set, defaults to the time the file was modified.
+    # When not 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__)
@@ -67,5 +112,26 @@ def self.lookup_ancestors
       [self]
     end
     # :nocov:
+
+    module AttributesAreAlwaysTheInitializationData
+      def initialize(attributes = {})
+        @initialized_attributes = attributes && attributes.symbolize_keys
+        super
+      end
+
+      # Defaults to the downcased model name.
+      # This probably isn't a good default, since it's not a unique instance identifier,
+      # but that was the old behavior \_('-')_/
+      def id
+        @initialized_attributes.fetch(:id) { self.class.model_name.name && self.class.model_name.name.downcase }
+      end
+
+      # The only way to change the attributes of an instance is to directly mutate the attributes.
+      #
+      #   model.attributes[:foo] = :bar
+      def attributes
+        @initialized_attributes
+      end
+    end
   end
 end

test/action_controller/adapter_selector_test.rb

@@ -3,6 +3,14 @@
 module ActionController
   module Serialization
     class AdapterSelectorTest < ActionController::TestCase
+      Profile = poro_without_legacy_model_support(::Model) do
+        attributes :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/active_model_serializers/model_test.rb

@@ -20,7 +20,7 @@ def test_initialization_with_string_keys
     end
 
     def test_attributes_can_be_read_for_serialization
-      klass = Class.new(ActiveModelSerializers::Model) do
+      klass = poro_without_legacy_model_support do
         attributes :one, :two, :three
       end
       original_attributes = { one: 1, two: 2, three: 3 }
@@ -42,7 +42,7 @@ def test_attributes_can_be_read_for_serialization
     end
 
     def test_id_attribute_can_be_read_for_serialization
-      klass = Class.new(ActiveModelSerializers::Model) do
+      klass = poro_without_legacy_model_support do
         attributes :id, :one, :two, :three
       end
       self.class.const_set(:SomeTestModel, klass)

test/cache_test.rb

@@ -49,7 +49,7 @@ class InheritedRoleSerializer < RoleSerializer
       attribute :special_attribute
     end
 
-    class Comment < ::Model
+    Comment = poro_without_legacy_model_support(::Model) do
       attributes :body
       associations :post, :author
 

test/fixtures/poro.rb

@@ -1,3 +1,6 @@
+# The test poro superclass can't have the legacy behavior included because there's no simple way to remove it.
+# This lets us turn the legacy behavior on and off in the test context so that we can test it.
+ActiveModelSerializers::Model.attributes_are_always_the_initialization_data = false
 class Model < ActiveModelSerializers::Model
   FILE_DIGEST = Digest::MD5.hexdigest(File.open(__FILE__).read)
 

test/support/poro.rb

@@ -0,0 +1,15 @@
+module ActiveModelSerializersWithoutLegacyModelSupport
+  module_function
+
+  def poro_without_legacy_model_support(superklass = ActiveModelSerializers::Model, &block)
+    original_attributes_are_always_the_initialization_data = superklass.attributes_are_always_the_initialization_data
+    superklass.attributes_are_always_the_initialization_data = false
+    Class.new(superklass) do
+      class_exec(&block) if block
+    end
+  ensure
+    superklass.attributes_are_always_the_initialization_data = original_attributes_are_always_the_initialization_data
+  end
+end
+Minitest::Test.include ActiveModelSerializersWithoutLegacyModelSupport
+Minitest::Test.extend ActiveModelSerializersWithoutLegacyModelSupport

test/test_helper.rb

@@ -52,6 +52,8 @@ def silence_warnings
   end
 end
 
+require 'support/poro'
+
 require 'support/rails_app'
 
 # require "rails/test_help"