Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate ActiveModelSerializers::Model #1911

Closed
wants to merge 1 commit into from
Closed
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
2 changes: 2 additions & 0 deletions active_model_serializers.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -59,4 +59,6 @@ Gem::Specification.new do |spec|
spec.add_development_dependency 'grape', ['>= 0.13', '< 1.0']
spec.add_development_dependency 'json_schema'
spec.add_development_dependency 'rake', ['>= 10.0', '< 12.0']
spec.add_development_dependency 'pry-byebug'
spec.add_development_dependency 'm'
Copy link
Member

Choose a reason for hiding this comment

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

I don't like putting anything in the gemspec not required for running the tests. I'd rather this be proposed to the Gemfile in a separate PR. Since this has come up a couple of times and I basically have the same thing in my Gemfile.local I think it's worth adding

Copy link
Contributor Author

Choose a reason for hiding this comment

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

end
16 changes: 15 additions & 1 deletion lib/active_model/serializer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -191,10 +191,24 @@ def json_key
end

def read_attribute_for_serialization(attr)
# Start by favoring serializers method override
if respond_to?(attr)
send(attr)
else
object.read_attribute_for_serialization(attr)
# Otherwise, check read_attribute_for_serialization
if object.respond_to?(:read_attribute_for_serialization)
object.read_attribute_for_serialization(attr)
Copy link
Member

Choose a reason for hiding this comment

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

The object must always respond to read_attribute_for_serialization. That's part of the contract. I'm not sure why we'd want to weaken that. I'd rather provide opt-in mixins for Hash for those that are trying to serialize a primitive (which you shouldn't be doing).

Copy link
Member

Choose a reason for hiding this comment

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

This is a huge philosophical break and also greatly expands the surface area that AMS needs to cover from a defined interface to a defined interface when available, else try to guess. I don't think we should support serializing primitives such as Hashes.

else
# Fall back to object property/method
if object.respond_to?(attr)
object.send(attr)
else
# Special logic for hashes
if object.is_a?(Hash)
Copy link
Contributor

Choose a reason for hiding this comment

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

👎

object[attr] || object[attr.to_s]
end
end
end
end
end

Expand Down
5 changes: 5 additions & 0 deletions lib/active_model_serializers.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,11 @@
require 'active_support/core_ext/object/with_options'
require 'active_support/core_ext/string/inflections'
require 'active_support/json'

require 'active_model_serializers/model_mixin'
Copy link
Member

Choose a reason for hiding this comment

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

proposal: active_model_serializers/serializable, or active_model_serializers/serializer_support similar to earlier apis 0.8 0.9

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 serializable


require 'pry-byebug'

module ActiveModelSerializers
extend ActiveSupport::Autoload
autoload :Model
Expand Down
40 changes: 11 additions & 29 deletions lib/active_model_serializers/model.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,23 +3,20 @@
# serializable non-activerecord objects.
module ActiveModelSerializers
class Model
include ActiveModel::Model
include ActiveModel::Serializers::JSON
include ActiveModelSerializers::ModelMixin

attr_reader :attributes, :errors
attr_reader :attributes

def initialize(attributes = {})
@attributes = attributes && attributes.symbolize_keys
@errors = ActiveModel::Errors.new(self)
super
end
def initialize(attrs = {})
@attributes = attrs && attrs.symbolize_keys

# Defaults to the downcased model name.
def id
attributes.fetch(:id) { self.class.name.downcase }
@attributes.each_pair do |key, value|
if respond_to?("#{key}=", value)
send("#{key}=", value)
end
end
end

# Defaults to the downcased model name and updated_at
def cache_key
attributes.fetch(:cache_key) { "#{self.class.name.downcase}/#{id}-#{updated_at.strftime('%Y%m%d%H%M%S%9N')}" }
end
Expand All @@ -29,23 +26,8 @@ def updated_at
attributes.fetch(:updated_at) { File.mtime(__FILE__) }
end

def read_attribute_for_serialization(key)
if key == :id || key == 'id'
attributes.fetch(key) { id }
else
attributes[key]
end
end

# The following methods are needed to be minimally implemented for ActiveModel::Errors
# :nocov:
def self.human_attribute_name(attr, _options = {})
attr
end

def self.lookup_ancestors
[self]
def id
attributes.fetch(:id) { self.class.name.downcase }
end
# :nocov:
end
end
57 changes: 57 additions & 0 deletions lib/active_model_serializers/model_mixin.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
# ActiveModelSerializers::Model is a convenient
# serializable class to inherit from when making
# serializable non-activerecord objects.
module ActiveModelSerializers
module ModelMixin
def self.included(klass)
klass.class_eval do
Copy link
Member

Choose a reason for hiding this comment

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

missing call to super in the included method

include ActiveModel::Model
include ActiveModel::Serializers::JSON

def read_attribute_for_serialization(key)
if key == :id || key == 'id'
send(key)
else
if is_a?(ActiveModelSerializers::Model)
# Support legacy behavior
attributes[key] || (send(key) if respond_to?(key))
else
send(key) if respond_to?(key)
end
end
end
end
end

#def id
#self.class.name.downcase
#end

#def cache_key
#end

#def errors
#@errors ||= ActiveModel::Errors.new(self)
#end
Copy link
Member

Choose a reason for hiding this comment

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

obviously this is a wip, but can't must remove these. They're part of the contract


#def updated_at
#end

# This is just to make lint pass
# We shouldnt have this...
#def attributes
#{}
#end

# The following methods are needed to be minimally implemented for ActiveModel::Errors
# :nocov:
#def self.human_attribute_name(attr, _options = {})
#attr
#end

#def self.lookup_ancestors
#[self]
#end
# :nocov:
end
end
2 changes: 1 addition & 1 deletion test/action_controller/json_api/fields_test.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
require 'test_helper'

require 'pry-byebug'
module ActionController
module Serialization
class JsonApi
Expand Down
26 changes: 26 additions & 0 deletions test/active_model_serializers/model_mixin_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
require 'test_helper'

module ActiveModelSerializers
class ModelMixinTest < ActiveSupport::TestCase
def setup
@klass = Class.new do
#include ActiveModelSerializers::ModelMixin
attr_accessor :key, :id
def self.name
'TestModel'
end
end
end

def test_poro_serialize
serializer = Class.new(ActiveModel::Serializer) do
attributes :key
end
model_instance = @klass.new
model_instance.key = 'value'

json = serializer.new(model_instance, {}).as_json
assert_equal({ key: 'value' }, json)
end
end
end
66 changes: 61 additions & 5 deletions test/active_model_serializers/model_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,17 +6,73 @@ class ModelTest < ActiveSupport::TestCase

def setup
@resource = ActiveModelSerializers::Model.new

@klass = Class.new(ActiveModelSerializers::Model) do
attr_accessor :key

def self.name
'TestModel'
end
end
end

def test_initialization_with_string_keys
klass = Class.new(ActiveModelSerializers::Model) do
attr_accessor :key
model_instance = @klass.new('key' => 'value')

assert_equal 'value', model_instance.read_attribute_for_serialization(:key)
end

def test_direct_accessor_assignment
model_instance = @klass.new
model_instance.key = 'value'
assert_equal 'value', model_instance.read_attribute_for_serialization(:key)
end

def test_fetch_id_from_attributes
model_instance = @klass.new(id: 1)
assert_equal 1, model_instance.id
end

# Note: 'id' defined by attr_accessor
# is mutually exclusive from default id behavior
# IOW, defined attr_accessors 'win'
def test_id_from_accessor
@klass.class_eval do
attr_accessor :id
end
value = 'value'
model_instance = @klass.new
model_instance.id = 1
assert_equal 1, model_instance.id
end

model_instance = klass.new('key' => value)
# not needed, out superclass
#def test_id_as_defined_method
#@klass.class_eval do
#def id
#@id
#end

#def id=(val)
#@id = val
#end
#end

#model_instance = @klass.new
#model_instance.id = 1
#assert_equal 1, model_instance.id
#end

# Note: This does not work if @klass defines
# attr_accessor :id
def test_default_id
model_instance = @klass.new
assert_equal 'testmodel', model_instance.id
end

assert_equal model_instance.read_attribute_for_serialization(:key), value
def test_errors
model_instance = @klass.new
model_instance.errors.add(:key, 'is blank')
assert_equal true, model_instance.errors[:key].present?
end
end
end
2 changes: 2 additions & 0 deletions test/adapter/json_api/resource_identifier_test.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
require 'test_helper'

require 'pry-byebug'

module ActiveModelSerializers
module Adapter
class JsonApi
Expand Down
4 changes: 3 additions & 1 deletion test/serializers/read_attribute_for_serialization_test.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
require 'test_helper'
require 'pry-byebug'

module ActiveModel
class Serializer
Expand Down Expand Up @@ -51,7 +52,8 @@ def success
end

def test_child_serializer_with_error_attribute
error = ErrorResponse.new(error: 'i have an error')
error = ErrorResponse.new
error.error = 'i have an error'
serializer = ErrorResponseSerializer.new(error)
serializer_with_super = ErrorResponseWithSuperSerializer.new(error)
assert_equal false, serializer.read_attribute_for_serialization(:status)
Expand Down