Skip to content
Merged
Show file tree
Hide file tree
Changes from 11 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
73 changes: 22 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
Copy link
Contributor

Choose a reason for hiding this comment

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

As we're already moving things around in this PR, shouldn't we use the opportunity to extract all caching-related methods/constants into its own module? It would make this file less confusing IMHO.

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 thought I did, but maybe that was adapters. I'm thinking I might back out the re-arranging to improve the focus in this PR

The rearranging is about moving the generic class functions away from the attribute/association mapping stuff but is a little distracting in the diff

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that's why I initially extracted the attributes stuff into serializer/attributes.rb (cleaner diff + cleaner files).

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 @@ -244,16 +208,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 _serializer_method_defined?(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.

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

Expand All @@ -266,5 +227,15 @@ def links
protected

attr_accessor :instance_options

private

def _serializer_instance_methods
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't that be better as a class method performance wise?

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 you're right. I was working on the instance-level and got tunnel-vision

@_serializer_instance_methods ||= (public_methods - Object.public_instance_methods).to_set
end

def _serializer_method_defined?(name)
_serializer_instance_methods.include?(name)
end
end
end
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
109 changes: 109 additions & 0 deletions lib/active_model/serializer/attributes.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
module ActiveModel
class Serializer
module Attributes
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
class AttributeReader < Attribute
def initialize(name)
super(name)
@reader = ->(instance) { instance.read_attribute_for_serialization(name) }
end
end
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
25 changes: 23 additions & 2 deletions lib/active_model/serializer/reflection.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,28 @@ 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

def value(instance)
call(instance)
end

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.instance_eval(&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.

I'm looking at 0.8 and how JSON API considers associations 'includes' and am thinking that association block should be applied to the association, rather than the serializer. Otherwise, there's really nothing that ties the block to an association.

So, usage from current impl would be

has_many :comments do
-  comments.limit(5)
+  limit(5)
end

which is also more inline with how the scope method works in Rails. I think this makes the api a lot more clear. If you want a custom attribute, use the attribute block. If you want a scoped association, I think it the instance_eval should be on the association.

See

      def associate(klass, attrs) #:nodoc:
        options = attrs.extract_options!
        self._associations = _associations.dup

        attrs.each do |attr|
          unless method_defined?(attr)
            define_method attr do
              object.send attr
            end
          end

          define_include_method attr

          self._associations[attr] = klass.refine(attr, options)
        end
      end

      def define_include_method(name)
        method = "include_#{name}?".to_sym

        INCLUDE_METHODS[name] = method

        unless method_defined?(method)
          define_method method do
            true
          end
        end
      end
    def include_associations!
      _associations.each_key do |name|
        include!(name) if include?(name)
      end
    end

    def include?(name)
      return false if @options.key?(:only) && !Array(@options[:only]).include?(name)
      return false if @options.key?(:except) && Array(@options[:except]).include?(name)
      send INCLUDE_METHODS[name]
    end

    def include!(name, options={})
      EMBED_IN_ROOT_OPTIONS = [
        :include,
      ]
    def associations(options={})
      associations = self.class._associations
      included_associations = filter(associations.keys)
      associations.each_with_object({}) do |(name, association), hash|
        if included_associations.include? name
        # ...
  end
    def filter(keys)
      if @only
        keys & @only
      elsif @except
        keys - @except
      else
        keys
      end
    end

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 +61,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
29 changes: 29 additions & 0 deletions test/serializers/associations_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,35 @@ def test_associations_custom_keys
assert expected_association_keys.include? :site
end

class InlineAssociationTestPostSerializer < ActiveModel::Serializer
has_many :comments
has_many :last_comments do
object.comments.last(1)
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a great example @beauby made of why you'd want to define your own association value

However, the way that this works, it's really the same as attribute :last_comments do end, right? Unless this affects deserialization, right?

end
end

def test_virtual_attribute_block
comment1 = ::ARModels::Comment.create!(contents: 'first comment')
comment2 = ::ARModels::Comment.create!(contents: 'last comment')
post = ::ARModels::Post.create!(
title: 'inline association test',
body: 'etc',
comments: [comment1, comment2]
)
actual = serializable(post, adapter: :attributes, serializer: InlineAssociationTestPostSerializer).as_json
expected = {
:comments => [
{ :id => 1, :contents => 'first comment' },
{ :id => 2, :contents => 'last comment' }
],
:last_comments => [
{ :id => 2, :contents => 'last comment' }
]
}

assert_equal expected, actual
end

class NamespacedResourcesTest < Minitest::Test
class ResourceNamespace
Post = Class.new(::Model)
Expand Down
Loading