Make serializer interface more obvious by bf4 · Pull Request #2074 · rails-api/active_model_serializers

bf4 · 2017-03-13T03:14:59Z

Remove unnecessary concerns that make understanding the Serializer just that much harder

Inspired by working with this stuff in #2026

I started some work to more completely separate out the caching code... but it's not ready.

mention-bot · 2017-03-13T03:15:00Z

@bf4, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bolshakov, @beauby and @NullVoxPopuli to be potential reviewers.

NullVoxPopuli · 2017-03-13T09:55:39Z

👍

bf4 · 2017-03-13T14:10:15Z

+    # @return [void]
+    #
+    # @api private
+    #


bf4 · 2017-03-13T14:12:53Z

+      Enumerator.new do |y|
+        self.class._reflections.values.each do |reflection|
+          next if reflection.excluded?(self)
+          key = reflection.options.fetch(:key, reflection.name)


NOTE: key should always be set since

def self.associate(reflection) key = reflection.options[:key] || reflection.name self._reflections[key] = reflection end

which mean we could safely change .values.each do |reflection| to .each do |key, reflection|

bf4 · 2017-03-13T14:13:17Z

+    #
+    def associations(include_directive = ActiveModelSerializers.default_include_directive, include_slice = nil)
+      include_slice ||= include_directive
+      return unless object


❓ For another PR, should return Enumerator.new, right?

yeah, it should be lazy, cause depending on the request / include args, maybe associations is never evaluated

bf4 · 2017-03-13T14:15:02Z

+    #     type 'authors'
+    def self.type(type)
+      self._type = type && type.to_s
+    end


# END SERIALIZER MACROS

bf4 · 2017-03-13T14:15:17Z

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


# BEGIN SERIALIZER MACROS

bf4 · 2017-03-13T14:15:49Z

+    # maps attribute value to explicit key name
+    # @see Serializer::attribute
+    # @see ActiveModel::Serializer::Caching#fragmented_attributes
+    def self._attributes_keys


TODO in another PR:

Comment out of date

Should be in the 'caching' concern. That's the only place it is used.

(Also, is not a 'MACRO')

bf4 · 2017-03-13T14:16:54Z

+    # @api private
+    # keys of attributes
+    # @see Serializer::attribute
+    def self._attributes


This should really be a public API, I think (Also, is not a 'MACRO')

bf4 · 2017-03-13T14:20:07Z

+
+    # Define a link on a serializer.
+    # @example
+    #   link(:self) { resource_url(object) }


NOTE: (for future PR)

resource_url will only work in the JSONAPI adapter, else need https://github.com/rails-api/active_model_serializers/blob/2a6d373cb269bd704d201fc0ff8e6c4255865c2c/docs/howto/add_relationship_links.md#links-as-an-attribute-of-a-resource

bf4 · 2017-03-13T14:22:47Z

+      key = reflection.options[:key] || reflection.name
+      self._reflections[key] = reflection
+    end
+    private_class_method :associate


NOTE for future PR:

doc how to configure default inclusion behavior: all associations, some associations, some fields in the associations, etc

Provide a clear way to include nested associations and fields #1849 (comment)

Include option for associations in serializers #1333 (comment)

bf4 · 2017-03-13T14:24:33Z

+    config.collection_serializer = ActiveModel::Serializer::CollectionSerializer
+    config.serializer_lookup_enabled = true
+
+    def config.array_serializer=(collection_serializer)


# @api deprecated

are you gonna add these deprecation tags?

bf4 · 2017-03-13T14:24:38Z

+      self.collection_serializer = collection_serializer
+    end
+
+    def config.array_serializer


# @api deprecated

bf4 · 2017-03-13T14:29:24Z

+    # Make JSON API top-level jsonapi member opt-in
+    # ref: http://jsonapi.org/format/#document-top-level
+    config.jsonapi_include_toplevel_object = false
+    config.include_data_default = true


TODO: DOC, is the default for the include_data reflection option, ref https://github.com/rails-api/active_model_serializers/blob/ff27032720956f7c5cd9ef50618606cfebaa7ad9/docs/general/serializers.md#associations

true (default)-- always include association data

false -- never include association data

:if_sideloaded (uses include_slice option passed to associations method, tl;dr include_slice.key?(reflection.name))

rafaelfranca · 2017-03-13T23:45:44Z

LGTM so far

bf4 · 2017-03-15T02:24:31Z

Updated

bf4 · 2017-03-15T14:03:50Z

    end

+    # Configuration options may also be set in
+    # Serializers and Adapters


❗️ comment appears to be out of date.

In addition, This is a reminder that some of the configuration stuff in here is 'serializer'-specific, and some is global, which is why I moved the preferred interface to ActiveModelSerializers.config from ActiveModel::Serializer.config, but I think there's an argument that some aspects of inheritable config should remain in the serializer.

bf4 · 2017-03-15T14:06:36Z

+    # Configuration options may also be set in
+    # Serializers and Adapters
+    config.collection_serializer = ActiveModel::Serializer::CollectionSerializer
+    config.serializer_lookup_enabled = true


Noting open issues with this:

Explicit serializer_lookup_enabled in controller doesn't retain Adapter for entire render #1500

[WIP] Allow users to opt-out from the ActionController extensions #1636

Setting serializer option to nil still using default serializer. #2014

Possible regression in 0.10.1 - Object instance variable in parent serializer set to serializer in 0.10.1 #1813 esp Possible regression in 0.10.1 - Object instance variable in parent serializer set to serializer in 0.10.1 #1813 (comment)

bf4 · 2017-03-15T14:08:31Z

+      collection_serializer
+    end
+
+    config.default_includes = '*'


Noting needed documentation:

https://github.com/rails-api/active_model_serializers/pulls?q=is%3Aopen+is%3Apr+label%3A%22C%3A+Deep+Nesting%22 and https://github.com/rails-api/active_model_serializers/pulls?q=is%3Aopen+is%3Aissue+label%3A%22C%3A+Deep+Nesting%22

bf4 · 2017-03-15T14:09:03Z

+    config.key_transform = nil
+    config.jsonapi_pagination_links_enabled = true
+    config.jsonapi_resource_type = :plural
+    config.jsonapi_namespace_separator = '-'.freeze


I was thinking... though we recommend -- now, why not .?

bf4 · 2017-03-15T14:13:05Z

+    end
+
+    config.default_includes = '*'
+    config.adapter = :attributes


See Adapter unification RFC and the difference between 'attributes' and 'json', in terms of serializing individual records, should be whether include_root_in_json or root is true. In terms of generating the response document, when root is true, then other options like meta could be processed; Otherwise, they should be ignored since there's no notion of a document (distinct from the serialized resource(s)) without a root, and hence no document-level meta.

bf4 · 2017-03-15T14:14:43Z

+    # @see Serializer::attribute
+    def self._attributes
+      _attributes_data.keys
+    end


I moved this above the 'MACROS' section and removed @api private since it's been a public interface across multiple versions. I think we tagged it as private just to be conservative and let us pick a different method name, if we like.

Too bad _attributes returns an array of attribute names and _reflections returns a hash of association names to reflections (is more like _attributes_data)

bf4 · 2017-03-15T14:17:54Z

+    # @example
+    #  has_many :comments, serializer: CommentSummarySerializer
+    #
+    def self.has_many(name, options = {}, &block) # rubocop:disable Style/PredicateName


making blocks available to associations complicates the reflection since there's no way to know anything about what's in it until it's evaluated, and hence trying to just get the id/type for a belongs_to, and not loading the association, is harder. We should probably revisit what information we have when a block is passed.

See #1857 (and #2026 )

bf4 · 2017-03-15T14:20:54Z

+    #     meta do
+    #       { comment_count: object.comments.count }
+    #     end
+    def self.meta(value = nil, &block)


It should be noted that this is a serializer-level (resource object) meta, distinct from adapter-level (document)

bf4 · 2017-03-15T14:25:21Z

+    #   class AdminAuthorSerializer < ActiveModel::Serializer
+    #     type 'authors'
+    def self.type(type)
+      self._type = type && type.to_s


resource roots esp as render options need review C: JSON Roots

Some thoughts I wrote in our Slack last September:

unifying the root key logic would be a huge boon,

from 1843 #1756 (comment) #1794 (comment)

summary of the issues: there's four methods that determine the resource root. This is especially confusing in the collection serializer. stay away until you need to, but can be useful in seeing the touch point. small working pieces > everything. so, any progress can be a pr.

summary of the goal: the main difference between the attributes and json adapters is the root option. look through the issues and prs and you'll see the trouble handling this at the adapter causes. If root could be turned on or off in the attributes or json adapter the same way, they might even be able to be rejoined. (They were split by joaomdmoura , and I've since moved a lot of the logic into the serializer)

There's lots of little pieces of this that are individually easish to find and fix. The overall goal is to just make it simple.

Naming: serializer type macro should be used to determine the root name, when present. The name can be over-ridden at the instance level either by passing in root or by defining a method named root. json_key is a bad name and should be deprecated. root is also kind of a bad name... actual method should probably be _type or something instead of root so as to not conflict with other instance methods and also for better parity. There's a lot of decisions to be made here.

so, within this there's 1) whether to include a root 2) where do we determine whether to declare a root (option, serializer, config, adapter)? 3) what should the value of the root be

bf4 · 2017-03-16T15:16:42Z

+    # Preferred interface is ActiveModelSerializers.config
+    # BEGIN DEFAULT CONFIGURATION
+    config.collection_serializer = ActiveModel::Serializer::CollectionSerializer
+    config.serializer_lookup_enabled = true


Noting open issues with this:

Explicit serializer_lookup_enabled in controller doesn't retain Adapter for entire render #1500

[WIP] Allow users to opt-out from the ActionController extensions #1636

Setting serializer option to nil still using default serializer. #2014

Possible regression in 0.10.1 - Object instance variable in parent serializer set to serializer in 0.10.1 #1813 esp Possible regression in 0.10.1 - Object instance variable in parent serializer set to serializer in 0.10.1 #1813 (comment)

bf4 · 2017-03-16T15:17:00Z

    end

+    # Preferred interface is ActiveModelSerializers.config
+    # BEGIN DEFAULT CONFIGURATION


This is a reminder that some of the configuration stuff in here is 'serializer'-specific, and some is global, which is why I moved the preferred interface to ActiveModelSerializers.config from ActiveModel::Serializer.config, but I think there's an argument that some aspects of inheritable config should remain in the serializer.

bf4 · 2017-03-16T15:17:40Z

+    # @see Serializer::attribute
+    def self._attributes
+      _attributes_data.keys
+    end


I moved this above the 'MACROS' section and removed @api private since it's been a public interface across multiple versions. I think we tagged it as private just to be conservative and let us pick a different method name, if we like.

Too bad _attributes returns an array of attribute names and _reflections returns a hash of association names to reflections (is more like _attributes_data)

bf4 · 2017-03-16T15:18:05Z

I think this is ready for merge

bf4 requested review from NullVoxPopuli, beauby, guilleiguaran, kurko and rafaelfranca March 13, 2017 13:50

bf4 added C: Cleanup S: Needs Code Review V: 0.10.x labels Mar 13, 2017

bf4 commented Mar 13, 2017

View reviewed changes

Comment thread lib/active_model/serializer.rb Outdated

# @return [void]

#

# @api private

#

Copy link
Copy Markdown

Member Author

bf4 Mar 13, 2017

Choose a reason for hiding this comment

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

✂️

bf4 commented Mar 13, 2017

View reviewed changes

Make serializer interface more obvious

36b4eac

bf4 force-pushed the make_serializer_interface_more_obvious branch from cb5abd0 to 36b4eac Compare March 15, 2017 01:55

bf4 added the S: Changelog Missing label Mar 15, 2017

bf4 commented Mar 15, 2017

View reviewed changes

Improve comments; move caching concern to caching.rb

2e71bc4

bf4 commented Mar 16, 2017

View reviewed changes

rafaelfranca approved these changes Mar 17, 2017

View reviewed changes

bf4 merged commit 80e470d into rails-api:master Mar 19, 2017

bf4 deleted the make_serializer_interface_more_obvious branch March 19, 2017 03:41

Conversation

bf4 commented Mar 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mention-bot commented Mar 13, 2017

Uh oh!

NullVoxPopuli commented Mar 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bf4 Mar 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rafaelfranca commented Mar 13, 2017

Uh oh!

bf4 commented Mar 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bf4 Mar 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bf4 Mar 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bf4 Mar 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bf4 Mar 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bf4 Mar 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bf4 commented Mar 16, 2017

Uh oh!

Reviewers

Assignees

bf4 commented Mar 13, 2017 •

edited

Loading

bf4 Mar 13, 2017 •

edited

Loading

bf4 Mar 15, 2017 •

edited

Loading

bf4 Mar 15, 2017 •

edited

Loading

bf4 Mar 15, 2017 •

edited

Loading

bf4 Mar 15, 2017 •

edited

Loading

bf4 Mar 16, 2017 •

edited

Loading