Skip to content

Add inline syntax to override attributes/associations. #1262

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

Add inline syntax to override attributes/associations. #1262

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0c4e1f5
Add inline syntax to override attributes.
beauby Oct 9, 2015
8c9f706
Refactor attributes.
beauby Oct 19, 2015
af8fc7e
Clean-up inheritance of attributes and associations.
beauby Oct 25, 2015
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
55 changes: 3 additions & 52 deletions lib/active_model/serializer.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
require 'active_model/serializer/collection_serializer'
require 'active_model/serializer/array_serializer'
require 'active_model/serializer/include_tree'
require 'active_model/serializer/attributes'
require 'active_model/serializer/associations'
require 'active_model/serializer/configuration'
require 'active_model/serializer/fieldset'
Expand All @@ -12,7 +13,9 @@
module ActiveModel
class Serializer
include Configuration
include Attributes
include Associations

require 'active_model/serializer/adapter'

# Matches
Expand Down Expand Up @@ -46,10 +49,6 @@ def self.digest_caller_file(caller_line)

with_options instance_writer: false, instance_reader: false do |serializer|
class_attribute :_type, instance_reader: true
class_attribute :_attributes # @api private : names of attribute methods, @see Serializer#attribute
self._attributes ||= []
class_attribute :_attributes_keys # @api private : maps attribute value to explict key name, @see Serializer#attribute
self._attributes_keys ||= {}
serializer.class_attribute :_cache # @api private : the cache object
serializer.class_attribute :_fragmented # @api private : @see ::fragmented
serializer.class_attribute :_cache_key # @api private : when present, is first item in cache_key
Expand All @@ -66,12 +65,9 @@ def self.digest_caller_file(caller_line)
serializer.class_attribute :_cache_digest # @api private : Generated
end

# Serializers inherit _attributes and _attributes_keys.
# Generates a unique digest for each serializer at load.
def self.inherited(base)
caller_line = caller.first
base._attributes = _attributes.dup
base._attributes_keys = _attributes_keys.dup
base._cache_digest = digest_caller_file(caller_line)
super
end
Expand All @@ -83,37 +79,6 @@ def self.type(type)
self._type = type
end

# @example
# class AdminAuthorSerializer < ActiveModel::Serializer
# attributes :id, :name, :recent_edits
def self.attributes(*attrs)
attrs = attrs.first if attrs.first.class == Array

attrs.each do |attr|
attribute(attr)
end
end

# @example
# class AdminAuthorSerializer < ActiveModel::Serializer
# attributes :id, :recent_edits
# attribute :name, key: :title
#
# def recent_edits
# object.edits.last(5)
# enr
def self.attribute(attr, options = {})
key = options.fetch(:key, attr)
_attributes_keys[attr] = { key: key } if key != attr
_attributes << key unless _attributes.include?(key)

ActiveModelSerializers.silence_warnings do
define_method key do
object.read_attribute_for_serialization(attr)
end unless method_defined?(key) || _fragmented.respond_to?(attr)
end
end

# @api private
# Used by FragmentCache on the CachedSerializer
# to call attribute methods on the fragmented cached serializer.
Expand Down Expand Up @@ -236,20 +201,6 @@ def json_key
root || object.class.model_name.to_s.underscore
end

# Return the +attributes+ of +object+ as presented
# by the serializer.
def attributes
attributes = self.class._attributes.dup

attributes.each_with_object({}) do |name, hash|
if self.class._fragmented
hash[name] = self.class._fragmented.public_send(name)
else
hash[name] = send(name)
end
end
end

protected

attr_accessor :instance_options
Expand Down
29 changes: 17 additions & 12 deletions lib/active_model/serializer/associations.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,8 @@ module Associations
DEFAULT_INCLUDE_TREE = ActiveModel::Serializer::IncludeTree.from_string('*')

included do |base|
class << base
attr_accessor :_reflections
end
base.class_attribute :_reflections
Copy link
Member

Choose a reason for hiding this comment

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

did you intend to allow instance reader/writers? did you intend to make it inheritable?

base._reflections ||= []

extend ActiveSupport::Autoload
autoload :Association
Expand All @@ -28,8 +27,10 @@ class << base
end

module ClassMethods
# Serializers inherit _reflections.
def inherited(base)
base._reflections = self._reflections.try(:dup) || []
super
base._reflections = _reflections.dup
end

# @param [Symbol] name of the association
Expand All @@ -39,8 +40,8 @@ def inherited(base)
# @example
# has_many :comments, serializer: CommentSummarySerializer
#
def has_many(name, options = {})
associate HasManyReflection.new(name, options)
def has_many(name, options = {}, &block)
associate(HasManyReflection.new(name, options), block)
end

# @param [Symbol] name of the association
Expand All @@ -50,8 +51,8 @@ def has_many(name, options = {})
# @example
# belongs_to :author, serializer: AuthorSerializer
#
def belongs_to(name, options = {})
associate BelongsToReflection.new(name, options)
def belongs_to(name, options = {}, &block)
associate(BelongsToReflection.new(name, options), block)
end

# @param [Symbol] name of the association
Expand All @@ -61,8 +62,8 @@ def belongs_to(name, options = {})
# @example
# has_one :author, serializer: AuthorSerializer
#
def has_one(name, options = {})
associate HasOneReflection.new(name, options)
def has_one(name, options = {}, &block)
associate(HasOneReflection.new(name, options), block)
end

private
Expand All @@ -73,11 +74,15 @@ def has_one(name, options = {})
#
# @api private
#
def associate(reflection)
def associate(reflection, block)
self._reflections = _reflections.dup

define_method reflection.name do
object.send reflection.name
if block_given?
instance_eval(&block)
else
object.send reflection.name
end
end unless method_defined?(reflection.name)

self._reflections << reflection
Expand Down
80 changes: 80 additions & 0 deletions lib/active_model/serializer/attributes.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
module ActiveModel
class Serializer
# Defines the attributes to be serialized.
#
module Attributes
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This module is directly extracted from the serializer, no modifications to the code have been made, except for:

  1. the definition of an inherit_attributes method, rather than overriding inherited as in the Associations module, which made it impossible for the two modules to define the inherited properties
  2. the optional block parameter to the attribute method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed the situation. Reverted to overriding inherited at module-level, but calling super each time in order to decorate the serializers. cc @bf4

extend ActiveSupport::Concern

included do |base|
base.class_attribute :_attributes # @api private : names of attribute methods, @see #attribute
base._attributes ||= []
base.class_attribute :_attributes_keys # @api private : maps attribute value to explict key name, @see #attribute
base._attributes_keys ||= {}
end

module ClassMethods
# Serializers inherit _attributes and _attributes_keys.
def inherited(base)
super
base._attributes = _attributes.dup
base._attributes_keys = _attributes_keys.dup
end

# @example
# class AdminAuthorSerializer < ActiveModel::Serializer
# attributes :id, :name, :recent_edits
def attributes(*attrs)
attrs = attrs.first if attrs.first.class == Array

attrs.each do |attr|
attribute(attr)
end
end

# @example
# class AdminAuthorSerializer < ActiveModel::Serializer
# attributes :id, :recent_edits
# attribute :name, key: :title
#
# def recent_edits
# object.edits.last(5)
# end
#
# @example
# class AdminAuthorSerializer < ActiveModel::Serializer
# attribute :name do
# "#{object.first_name} #{object.last_name}"
# end
def attribute(attr, options = {}, &block)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the &block param.

key = options.fetch(:key, attr)
_attributes_keys[attr] = { key: key } if key != attr
_attributes << key unless _attributes.include?(key)

ActiveModelSerializers.silence_warnings do
define_method key do
Copy link
Contributor

Choose a reason for hiding this comment

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

this absolutely needs to be document above the method signature with some examples.
I am a HUUUGE fan of metaprogramming, but it can be a pain in the butt if people aren't expecting it. So, having some example syntax in a method yardoc would be fabulous.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, although this code is already in AMS. I just added the if block_given?; ...; else.

Copy link
Contributor

Choose a reason for hiding this comment

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

Never hurts to add documentation. Code pre-existing isn't an excuse to not add docs. :-\

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, as I said, I agree. I was just making clear that I didn't just throw loads of metaprogramming magic without comments.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh :-) 👍

Copy link
Member

Choose a reason for hiding this comment

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

  1. we should be conservative in adding new interfaces because then we need to support it
  2. if we need this feaure, why? If we don't need it, what are the trade offs of adding it? Not adding it?
  3. how does it affect ease of understand usage? Ease of maintenaince? Complexity of either?
  4. is it extending existing behavior or modifying it? Could it bd done another way?
  5. how do you see this feature evolving? What are its limitations? Can it be easily customized? Eg adapter no longer need to be in thd ams namespace or even inherit the base. So, it's easy to extend (though the boundaries could be clearer and the interfaces more consitent. Am::s should probably be called a resource and implement the serializers::json interface
    (Writing in phone; apologies for anything unclear)

if block_given?
instance_eval(&block)
else
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the if block_given?; instance_eval(&block); else clause.

object.read_attribute_for_serialization(attr)
end
end unless method_defined?(key) || _fragmented.respond_to?(attr)
end
end
end

# Return the +attributes+ of +object+ as presented
# by the serializer.
def attributes
attributes = self.class._attributes.dup

attributes.each_with_object({}) do |name, hash|
if self.class._fragmented
hash[name] = self.class._fragmented.public_send(name)
else
hash[name] = send(name)
end
end
end
end
end
end
15 changes: 15 additions & 0 deletions test/serializers/attribute_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,21 @@ def id

assert_equal('custom', hash[:blog][:id])
end

PostWithVirtualAttribute = Class.new(::Model)
class PostWithVirtualAttributeSerializer < ActiveModel::Serializer
attribute :name do
"#{object.first_name} #{object.last_name}"
end
end

def test_virtual_attribute_block
post = PostWithVirtualAttribute.new(first_name: 'Lucas', last_name: 'Hosseini')
Copy link
Member

Choose a reason for hiding this comment

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

in this case, you might want to test post.attributes since that's the immediate case.. tests less layers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to see this too.

hash = serializable(post).serializable_hash
expected = { name: 'Lucas Hosseini' }

assert_equal(expected, hash)
end
end
end
end