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

Add inline syntax for attributes and associations #1356

Merged
merged 14 commits into from
Dec 10, 2015
Merged
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
72 changes: 21 additions & 51 deletions lib/active_model/serializer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
require 'active_model/serializer/array_serializer'
require 'active_model/serializer/include_tree'
require 'active_model/serializer/associations'
require 'active_model/serializer/attributes'
require 'active_model/serializer/configuration'
require 'active_model/serializer/fieldset'
require 'active_model/serializer/lint'
Expand All @@ -13,6 +14,7 @@ module ActiveModel
class Serializer
include Configuration
include Associations
include Attributes
require 'active_model/serializer/adapter'

# Matches
Expand Down Expand Up @@ -45,14 +47,9 @@ def self.digest_caller_file(caller_line)
end

with_options instance_writer: false, instance_reader: false do |serializer|
Copy link
Member Author

Choose a reason for hiding this comment

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

not sure why these weren't serializer.class_attribute

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it actually different?

Copy link
Member Author

Choose a reason for hiding this comment

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

I suppose it would be easy enough to find out, but I didn't...

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 ||= {}
class_attribute :_links # @api private : links definitions, @see Serializer#link
serializer.class_attribute :_type, instance_reader: true
serializer.class_attribute :_links # @api private : links definitions, @see Serializer#link
self._links ||= {}

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 @@ -69,12 +66,10 @@ def self.digest_caller_file(caller_line)
serializer.class_attribute :_cache_digest # @api private : Generated
end

# Serializers inherit _attributes and _attributes_keys.
# Serializers inherit _attribute_mappings, _reflections, and _links.
# 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._links = _links.dup
base._cache_digest = digest_caller_file(caller_line)
super
Expand All @@ -91,37 +86,6 @@ def self.link(name, value = nil, &block)
_links[name] = block || value
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 @@ -220,6 +184,15 @@ def self.get_serializer_for(klass)
end
end

def self._serializer_instance_method_defined?(name)
_serializer_instance_methods.include?(name)
end

def self._serializer_instance_methods
@_serializer_instance_methods ||= (public_instance_methods - Object.public_instance_methods).to_set
end
private_class_method :_serializer_instance_methods

attr_accessor :object, :root, :scope

# `scope_name` is set as :current_user by default in the controller.
Expand All @@ -244,16 +217,13 @@ def json_key
root || object.class.model_name.to_s.underscore
end

# Return the +attributes+ of +object+ as presented
# by the serializer.
def attributes(requested_attrs = nil)
self.class._attributes.each_with_object({}) do |name, hash|
next unless requested_attrs.nil? || requested_attrs.include?(name)
if self.class._fragmented
hash[name] = self.class._fragmented.public_send(name)
else
hash[name] = send(name)
end
def read_attribute_for_serialization(attr)
Copy link
Member Author

Choose a reason for hiding this comment

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

I think this reads pretty well and doesn't need documentation

if self.class._serializer_instance_method_defined?(attr)
send(attr)
elsif self.class._fragmented
self.class._fragmented.read_attribute_for_serialization(attr)
else
object.read_attribute_for_serialization(attr)
end
end

Expand Down
26 changes: 12 additions & 14 deletions lib/active_model/serializer/associations.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,10 @@ module Associations

DEFAULT_INCLUDE_TREE = ActiveModel::Serializer::IncludeTree.from_string('*')

included do |base|
class << base
attr_accessor :_reflections
included do
with_options instance_writer: false, instance_reader: true do |serializer|
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know if there's a difference between the block variables base and serializer in what we want here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Unclear to me as well.

serializer.class_attribute :_reflections
self._reflections ||= []
end

extend ActiveSupport::Autoload
Expand All @@ -29,7 +30,8 @@ class << base

module ClassMethods
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 +41,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 +52,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 +63,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 @@ -76,10 +78,6 @@ def has_one(name, options = {})
def associate(reflection)
self._reflections = _reflections.dup
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this dup is useless. There is no test breaking when I remove it, and I can't think of any case where it would be necessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, It doesn't seem to make sense, but it's pre-existing, and there's enough changes in here already

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed.


define_method reflection.name do
object.send reflection.name
end unless method_defined?(reflection.name)

self._reflections << reflection
end
end
Expand Down
112 changes: 112 additions & 0 deletions lib/active_model/serializer/attributes.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
module ActiveModel
class Serializer
module Attributes
# @api private
class Attribute
delegate :call, to: :reader

attr_reader :name, :reader

def initialize(name)
@name = name
@reader = :no_reader
end

def self.build(name, block)
if block
AttributeBlock.new(name, block)
else
AttributeReader.new(name)
end
end
end
# @api private
class AttributeReader < Attribute
def initialize(name)
super(name)
@reader = ->(instance) { instance.read_attribute_for_serialization(name) }
end
end
# @api private
class AttributeBlock < Attribute
def initialize(name, block)
super(name)
@reader = ->(instance) { instance.instance_eval(&block) }
end
end
Copy link
Contributor

Choose a reason for hiding this comment

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

I see what you did here but I have a feeling the whole Attribute / AttributeReader / AttributeBlock tryptic is a bit overcomplicated compared to what we actually want to achieve here: storing a proc built from either a block or an attribute name.

Copy link
Member Author

Choose a reason for hiding this comment

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

I like the polymorphism. Do you mean as opposed to just a conditional statement that returns the appropriate proc?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have anything against polymorphism in general, but I don't really find it necessary here. This whole part of code could be replaced by a single method, and I am not sure the polymorphism really adds anything here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Would you mind showing me what you have in mind? I think this is 90% a style thing, at this point, and I just found this more readable and extendable. Having the Attribute object let me keep track of the name, which I needed to use, which is why I introduced it (as opposed to just wanting it there for a future need)

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure. I meant defining a ("private") class method as follows:

def _attribute_mapping(name, block)
  if block
    ->(instance) { instance.instance_eval(&block) }
  else
    ->(instance) { instance.read_attribute_for_serialization(name) }
  end
end

and the attribute method would just be

+        def attribute(attr, options = {}, &block)
+          key = options.fetch(:key, attr)
+          _attribute_mappings[key] = _attribute_mapping(attr, block)
+        end

Copy link
Member Author

Choose a reason for hiding this comment

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

That would be fine by me, except I'd prefer not to add another class method, and without a reference to name, I think we'd have to go back to the old way of defining _attributes_keys, no? All of which drive me to find polymorphic classes easier to reason about and use.

def attribute(name, options = {}, &block)
-      _attributes_keys[attr] = { key: key } if key != attr
end

+        # @api private
+        # maps attribute value to explict key name
+        # @see Serializer::attribute
+        # @see Adapter::FragmentCache#fragment_serializer
+        def _attributes_keys
+          _attribute_mappings
+            .each_with_object({}) do |(key, attribute_mapping), hash|
+              next if key == attribute_mapping.name
+              hash[attribute_mapping.name] = { key: key }
+            end

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you think it would make sense for you to make a PR into this branch so we can review it that way? Sounds like we're both ok with this, except a few not-too-major things

Copy link
Contributor

Choose a reason for hiding this comment

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

Will add a PR to this branch, we'll see how it works.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, but if you it takes too long, we'll just make that a PR against master, right?

ref of discussion:

Copy link
Member Author

Choose a reason for hiding this comment

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


extend ActiveSupport::Concern

included do
with_options instance_writer: false, instance_reader: false do |serializer|
serializer.class_attribute :_attribute_mappings # @api private : maps attribute key names to names to names of implementing methods, @see #attribute
self._attribute_mappings ||= {}
end

# Return the +attributes+ of +object+ as presented
# by the serializer.
def attributes(requested_attrs = nil, reload = false)
@attributes = nil if reload
@attributes ||= self.class._attribute_mappings.each_with_object({}) do |(key, attribute_mapping), hash|
next unless requested_attrs.nil? || requested_attrs.include?(key)
hash[key] = attribute_mapping.call(self)
end
end
end

module ClassMethods
def inherited(base)
super
base._attribute_mappings = _attribute_mappings.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
#
# attribute :full_name do
# "#{object.first_name} #{object.last_name}"
# end
#
# def recent_edits
# object.edits.last(5)
# end
def attribute(attr, options = {}, &block)
key = options.fetch(:key, attr)
_attribute_mappings[key] = Attribute.build(attr, block)
end

# @api private
# names of attribute methods
# @see Serializer::attribute
def _attributes
_attribute_mappings.keys
end

# @api private
# maps attribute value to explict key name
# @see Serializer::attribute
# @see Adapter::FragmentCache#fragment_serializer
def _attributes_keys
_attribute_mappings
.each_with_object({}) do |(key, attribute_mapping), hash|
next if key == attribute_mapping.name
hash[attribute_mapping.name] = { key: key }
end
end
end
end
end
end
35 changes: 33 additions & 2 deletions lib/active_model/serializer/reflection.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,16 @@ class Serializer
# class PostSerializer < ActiveModel::Serializer
# has_one :author, serializer: AuthorSerializer
# has_many :comments
# has_many :comments, key: :last_comments do
# last(1)
# end
# end
#
# Notice that the association block is evaluated in the context of the association.
# Specifically, the association 'comments' is evaluated two different ways:
# 1) as 'comments' and named 'comments'.
# 2) as 'comments.last(1)' and named 'last_comments'.
Copy link
Member Author

Choose a reason for hiding this comment

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

See comment https://github.com/rails-api/active_model_serializers/pull/1356/files#r46723777 about how we should think of association blocks (and how we might want to also consider them as only used when included under JSON API) and see #1325 (comment)

#
# PostSerializer._reflections #=>
# # [
# # HasOneReflection.new(:author, serializer: AuthorSerializer),
Expand All @@ -17,7 +25,30 @@ class Serializer
#
# So you can inspect reflections in your Adapters.
#
Reflection = Struct.new(:name, :options) do
Reflection = Struct.new(:name, :options, :block) do
delegate :call, to: :reader
Copy link
Contributor

Choose a reason for hiding this comment

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

Why delegating it rather than calling @reader.call in value?

Copy link
Member Author

Choose a reason for hiding this comment

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

I just think it reads better. It might be a performance issue.

I think there's definitely room for refactoring and revising this, but I don't think it should be a blocker. I don't really like how the reflection differs from the attribute, but figure we can kick that can down the road a bit more into its own PR.


attr_reader :reader

def initialize(*)
super
@reader = self.class.build_reader(name, block)
end

# @api private
def value(instance)
call(instance)
end

# @api private
def self.build_reader(name, block)
Copy link
Member Author

Choose a reason for hiding this comment

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

like AttributeBlock and AttributeReader, but needs to be in the Reflection rather than Association due to how the code is currently written, so I just build the reader in the initialize rather than use polymorphism

Copy link
Member Author

Choose a reason for hiding this comment

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

ref: https://github.com/rails-api/active_model_serializers/pull/1356/files#r46783854

I'm also not sure about 'value' as a name here... or even instance in place of serializer_instance or whatever.. just trying to get a feel for which names work better

if block
->(instance) { instance.read_attribute_for_serialization(name).instance_eval(&block) }
else
->(instance) { instance.read_attribute_for_serialization(name) }
end
end

# Build association. This method is used internally to
# build serializer's association by its reflection.
#
Expand All @@ -40,7 +71,7 @@ class Serializer
# @api private
#
def build_association(subject, parent_serializer_options)
association_value = subject.send(name)
association_value = value(subject)
reflection_options = options.dup
serializer_class = subject.class.serializer_for(association_value, reflection_options)

Expand Down
2 changes: 1 addition & 1 deletion test/fixtures/poro.rb
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ def custom_options
attributes :id, :name, :description, :slug

def slug
"#{name}-#{id}"
"#{object.name}-#{object.id}"
Copy link
Member Author

Choose a reason for hiding this comment

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

Possible regression, see #1356 (comment)

end

belongs_to :author
Expand Down
Loading