Skip to content

Add ActiveModel::Serializer#default_include #1845

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 7 commits into from
Closed

Add ActiveModel::Serializer#default_include #1845

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ce57006
Merge pull request #1 from rails-api/master
iMacTia Jul 13, 2016
a6495fb
Added ActiveModel::Serializer#default_include class attribute to impl…
iMacTia Jul 14, 2016
04e88f3
added default value for IncludeDirective options
iMacTia Jul 14, 2016
de7b6f4
refactored #deafult_include, changed parameter name into include_args…
iMacTia Jul 14, 2016
4b6fafb
added #always_include class_attribute for Associations module
iMacTia Jul 14, 2016
a9744ea
tests refactoring using #serializable tests helper
iMacTia Jul 14, 2016
9cdbfaa
added assertions on the "type" of included resources
iMacTia Jul 14, 2016
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
7 changes: 5 additions & 2 deletions lib/active_model/serializer.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,13 +100,16 @@ def self.get_serializer_for(klass)

# @api private
def self.include_directive_from_options(options)
if options[:include_directive]
include_directive = if options[:include_directive]
options[:include_directive]
elsif options[:include]
JSONAPI::IncludeDirective.new(options[:include], allow_wildcard: true)
else
ActiveModelSerializers.default_include_directive
end
_default_include ?
include_directive.merge(_default_include) :
include_directive
end

# @api private
Expand Down Expand Up @@ -164,7 +167,7 @@ def success?
# serializer.as_json(include: { posts: { include: { comments: { only: :body } }, only: :title } })
def serializable_hash(adapter_options = nil, options = {}, adapter_instance = self.class.serialization_adapter_instance)
adapter_options ||= {}
options[:include_directive] ||= ActiveModel::Serializer.include_directive_from_options(adapter_options)
options[:include_directive] ||= self.class.include_directive_from_options(adapter_options)
cached_attributes = adapter_options[:cached_attributes] ||= {}
resource = fetch_attributes(options[:fields], cached_attributes, adapter_instance)
relationships = resource_relationships(adapter_options, options, adapter_instance)
Expand Down
36 changes: 34 additions & 2 deletions lib/active_model/serializer/associations.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ module Associations

included do
with_options instance_writer: false, instance_reader: true do |serializer|
serializer.class_attribute :_reflections
serializer.class_attribute :_reflections, :_default_include, :_always_include
self._reflections ||= []
end

Expand Down Expand Up @@ -65,6 +65,26 @@ def has_one(name, options = {}, &block) # rubocop:disable Style/PredicateName
associate(HasOneReflection.new(name, options, block))
end

# Set _default_include to the parsed value of +include_args+.
# @param include_args value to be parsed by JSONAPI::IncludeDirective::Parser
# @param options options for JSONAPI::IncludeDirective::Parser, default { allow_wildcard: true }
# @return [void]
#
def default_include(include_args, options = {})
default_options = { allow_wildcard: true }
self._default_include = JSONAPI::IncludeDirective.new(include_args, default_options.merge(options))
end

# Set _always_include to the parsed value of +include_args+.
# @param include_args value to be parsed by JSONAPI::IncludeDirective::Parser
# @param options options for JSONAPI::IncludeDirective::Parser, default { allow_wildcard: true }
# @return [void]
#
def always_include(include_args, options = {})
default_options = { allow_wildcard: true }
self._always_include = JSONAPI::IncludeDirective.new(include_args, default_options.merge(options))
end

private

# Add reflection and define {name} accessor.
Expand All @@ -74,17 +94,29 @@ def has_one(name, options = {}, &block) # rubocop:disable Style/PredicateName
# @api private
#
def associate(reflection)
self._reflections << reflection
_reflections << reflection
end
end

# Instance method to get _default_include
def default_include
_default_include
end

# Instance method to get _always_include
def always_include
_always_include
end

# @param [JSONAPI::IncludeDirective] include_directive (defaults to the
# +default_include_directive+ config value when not provided)
# @return [Enumerator<Association>]
#
def associations(include_directive = ActiveModelSerializers.default_include_directive)
return unless object

include_directive.merge!(always_include) if always_include

Enumerator.new do |y|
self.class._reflections.each do |reflection|
next if reflection.excluded?(self)
Expand Down
7 changes: 6 additions & 1 deletion lib/active_model_serializers/adapter/json_api.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,12 @@ def resource_objects_for(serializers)
@included = []
@resource_identifiers = Set.new
serializers.each { |serializer| process_resource(serializer, true) }
serializers.each { |serializer| process_relationships(serializer, @include_directive) }
serializers.each do |serializer|
include_directive = serializer.default_include ?
@include_directive.merge(serializer.default_include) :
@include_directive
process_relationships(serializer, include_directive)
end

[@primary, @included]
end
Expand Down
86 changes: 86 additions & 0 deletions test/serializers/always_include_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
require 'test_helper'

module ActiveModel
class Serializer
class DefaultIncludeTest < ActiveSupport::TestCase
class Blog < ActiveModelSerializers::Model
attr_accessor :id, :name, :posts
end
class Post < ActiveModelSerializers::Model
attr_accessor :id, :title, :author, :category
end
class Author < ActiveModelSerializers::Model
attr_accessor :id, :name
end
class Category < ActiveModelSerializers::Model
attr_accessor :id, :name
end

class BlogSerializer < ActiveModel::Serializer
always_include 'posts'
attributes :id
attribute :name, key: :title

has_many :posts
end
class PostSerializer < ActiveModel::Serializer
always_include 'author'
attributes :id, :title

has_one :author
has_one :category
end
class AuthorSerializer < ActiveModel::Serializer
attributes :id, :name
end
class CategorySerializer < ActiveModel::Serializer
attributes :id, :name
end

setup do
@authors = [Author.new(id: 1, name: 'Blog Author')]
@categories = [Category.new(id: 1, name: 'Food')]
@posts = [
Post.new(id: 1, title: 'The first post', author: @authors.first, category: @categories.first),
Post.new(id: 2, title: 'The second post', author: @authors.first, category: @categories.first)]
@blog = Blog.new(id: 2, name: 'The Blog', posts: @posts)
end

test '#always_include option populate "included" for json_api adapter' do
serialized = serializable(@blog, serializer: BlogSerializer, adapter: :json_api).as_json

assert_equal serialized[:included].size, 3
assert_equal 'The first post', serialized[:included].first[:attributes][:title]
types = serialized[:included].map { |h| h[:type] }
assert_includes(types, 'active-model-serializer-default-include-test-posts')
assert_includes(types, 'active-model-serializer-default-include-test-authors')
end

test '#always_include merges with the render include for json_api adapter' do
serialized = serializable(@blog, serializer: BlogSerializer, adapter: :json_api, include: 'posts.category').as_json

assert_equal serialized[:included].size, 4
assert_equal 'The first post', serialized[:included].first[:attributes][:title]
types = serialized[:included].map { |h| h[:type] }
assert_includes(types, 'active-model-serializer-default-include-test-posts')
assert_includes(types, 'active-model-serializer-default-include-test-authors')
assert_includes(types, 'active-model-serializer-default-include-test-categories')
end

test '#always_include option include associations for attributes adapter' do
serialized = serializable(@blog, serializer: BlogSerializer, adapter: :attributes).as_json

assert_equal serialized[:posts].size, 2
assert_equal 'Blog Author', serialized[:posts].first[:author][:name]
end

test '#always_include merges with the render include for attributes adapter' do
serialized = serializable(@blog, serializer: BlogSerializer, adapter: :attributes, include: 'posts.category').as_json

assert_equal serialized[:posts].size, 2
assert_equal 'Blog Author', serialized[:posts].first[:author][:name]
assert_equal 'Food', serialized[:posts].first[:category][:name]
end
end
end
end
86 changes: 86 additions & 0 deletions test/serializers/default_include_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
require 'test_helper'

module ActiveModel
class Serializer
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add a test for if a post has default_includes, but the object you're serializing does not?

I don't think we want default_includes to be 'respected' the whole way down the render chain

Copy link
Author

Choose a reason for hiding this comment

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

Sorry not sure I got this one. I think this is what I did.

Can you add a test for if a post has default_includes, but the object you're serializing does not?

default_include is specified in the PostSerializer, but in the test I'm serialising a Blog object.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, sorry. I guess I wrote too quickly. Here is there scenario I'd like tested:

ParentSerializer
  has_many Child

ChildSerializer
  default_include [:grand_child]

when serializing the parent with include: 'children' (this was probably a bad example, thanks English pluralization), only the child should show up, and not the grand child.

Copy link
Author

@iMacTia iMacTia Jul 14, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Ah in this case I think my current test is not gonna work.
I'm assuming in this case that serialising the Blog we'll see the authors in the result, which are in the default_include of PostSerializer.

This is because I'm using adapter: :attributes in the test, which by default include first level associations (the equivalent of having include: 'posts' in JSON-API adapter).

Also, my implementation actually "merges" the controller include with the serialiser one. Are you saying the controller one should actually override the serialiser's?

Maybe a quick "expectations" diagram would help in this case 😅.
Let's take my example of Blog -> Posts (default_include :author) -> Author

  1. What's the expected result for a simple render json: Blog.first?
  2. What's the expected result for render json: Blog.first, include: 'posts'?
  3. Let's assume posts have also another association "category", what's the expected result for render json: Blog.first, include: 'posts.category'?

Copy link
Contributor

@NullVoxPopuli NullVoxPopuli Jul 14, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

good clarification!
1.
data:

  • blog

  1. data:

    • blog

    included:

    • posts
      • I guess this could be up for debate, but I think for deeply nested includes, it should rely on the the controller include option, to reduce accidental / unavoidable data inclusion. i.e.: I think default_includes on only the top level serializable object allows for the most flexibility

  2. data:

    • blog

    include:

    • posts
    • category

Copy link
Contributor

Choose a reason for hiding this comment

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

no problem :-)

Two.) render json: Blog.first, include: 'posts'

data: 
  blog
included:
  posts
  authors

Three.) render json: Blog.first, include: 'posts.category'

data: 
  blog
included:
  posts
  authors
  categories

So, the include option is only merged with the default includes in the top-most serializer, in my perfect world, at least :-)

Copy link
Author

Choose a reason for hiding this comment

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

Ok, thanks @NullVoxPopuli.
My initial idea was to have a cascade-include behaviour, but I can see why you prefer going down this route. I'll probably need to move my code then because AFAIK associations method is called for each serialiser, even nested ones.
It would make more sense to have the merge happening when the ActiveModelSerializers::SerializableResource is created for rendering

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, though, the week this is released, I bet someone wants the cascading functionality you have currently.

So... idk. I'm not a huge fan of all the configuration options, but as long as there are thorough tests for them, I think It could be ok.

Copy link
Author

@iMacTia iMacTia Jul 14, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

It should be quite easy for me to implement both ways to do it.
The only question is: should I add an option on default_include (root_only, or always), or should I make another class attribute to support both of them 😃 ?

What about an always_include together with our new default_include?
The former would be taken in consideration for cascades, the latter only as first-level serialiser.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like always_include and default_include :-)

class DefaultIncludeTest < ActiveSupport::TestCase
class Blog < ActiveModelSerializers::Model
attr_accessor :id, :name, :posts
end
class Post < ActiveModelSerializers::Model
attr_accessor :id, :title, :author, :category
end
class Author < ActiveModelSerializers::Model
attr_accessor :id, :name
end
class Category < ActiveModelSerializers::Model
attr_accessor :id, :name
end

class BlogSerializer < ActiveModel::Serializer
default_include 'posts.author'
attributes :id
attribute :name, key: :title

has_many :posts
end
class PostSerializer < ActiveModel::Serializer
default_include 'author'
attributes :id, :title

has_one :author
Copy link
Member

Choose a reason for hiding this comment

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

I think I'd prefer 'has_one :author, include: :all' as a more declarative and easier to implement... I wrote more on this somewhere else... Advantage would include specifying included fields and smaller public api

Copy link
Contributor

Choose a reason for hiding this comment

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

but what does include all mean? include all dependencies? include all authors?

if anything, include: :always would make more sense.

Though, implementation is going to be slightly more complicated than the current.

Advantage would include specifying included fields

can you explain thoughts on this?

Copy link
Member

Choose a reason for hiding this comment

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

Quoting from my comment in #1843 (comment)

  • serializers have an attributes methods. It executes read_attribute_for_serialization on each attribute in self.class._attributes.
    • the list of serialized attributes can be limited by passing in an argument options[:fields]
  • serializers have a list of defined associations
    • for each associated, a serializer is found, and we call attributes on it.
      • we pass in to attributes options[:includes][association_name], see serializer for what this does, like fields

So, that

render options serializer attributes under JSON adapter serializer attributes under JSONAPI adapter
has_one :author none AuthorSerializer.new(object.author).attributes not serialized
has_one :author include: [:author] AuthorSerializer.new(object.author).attributes AuthorSerializer.new(object.author).attributes
has_one :author include: [:author], fields: { authors: [:name]} AuthorSerializer.new(object.author).attributes([:name]) AuthorSerializer.new(object.author).attributes([:name])
has_one :author, include: :all none AuthorSerializer.new(object.author).attributes AuthorSerializer.new(object.author).attributes
has_one :author, include: [:name] none AuthorSerializer.new(object.author).attributes([:name]) AuthorSerializer.new(object.author).attributes([:name])
has_one :author, include: :all include: [:author], fields: {authors: [:name]} AuthorSerializer.new(object.author).attributes([:name]) AuthorSerializer.new(object.author).attributes([:name])

Copy link
Member

Choose a reason for hiding this comment

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

@NullVoxPopuli

but what does include all mean? include all dependencies? include all authors?
if anything, include: :always would make more sense.

:all, meaning 'all fields', was really just an attempt at something better than true and something like for all attributes include: true or limit them with include: true, fields: [:name]. I think there's definitely a better way to phrase this, but main point is location of the new api and the kinds of words to use.

So that we could exchange a line in the controller

    render json: post, include: [:metrics, :media, :publishes, :author],
        fields: { media: [:title], publishers: (PublisherSerializer._attributes - :publisher_bio) }

with

class PostSerializer
  has_many :metrics, include: :all
  has_one :media, include: [:title]
  has_many :publishers, exclude: [:publisher_bio]
end
render json: post

Copy link
Author

Choose a reason for hiding this comment

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

I can see the advantage of the fields option, but wouldn't be easy to confuse include and exclude with only and except? Actually, aren't they doing the same thing?

Copy link
Author

Choose a reason for hiding this comment

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

Just as a background, include as an association option was already discussed on #1113, but finally dismissed in favour of a controller-level include.
There were many reasons for these (I strongly advise a quick read on the above issue), but it was mainly due to the confusion this could produce.

I think that default_include and always_include would provide a useful additional feature, without getting too much out of context and without creating confusion.

It's really just a simple way to keep code DRY and avoid copy-pasting includes in your controllers when the same serialiser is used.

This doesn't mean that we can't implement also an association-level include, I see it as a complementary feature, and not a substitution for this one 😄

has_one :category
end
class AuthorSerializer < ActiveModel::Serializer
attributes :id, :name
end
class CategorySerializer < ActiveModel::Serializer
attributes :id, :name
end

setup do
@authors = [Author.new(id: 1, name: 'Blog Author')]
@categories = [Category.new(id: 1, name: 'Food')]
@posts = [
Post.new(id: 1, title: 'The first post', author: @authors.first, category: @categories.first),
Post.new(id: 2, title: 'The second post', author: @authors.first, category: @categories.first)]
@blog = Blog.new(id: 2, name: 'The Blog', posts: @posts)
end

test '#default_include option populate "included" for json_api adapter' do
serialized = serializable(@blog, serializer: BlogSerializer, adapter: :json_api).as_json

assert_equal serialized[:included].size, 3
Copy link
Contributor

Choose a reason for hiding this comment

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

for both these tests and the always tests, I'd actually make sure that the correct types are rendered, just so whomever is looking at these tests in the future can very easily see what is being rendered.

Copy link
Author

Choose a reason for hiding this comment

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

Added a check for them in my latest commit 😄

assert_equal 'The first post', serialized[:included].first[:attributes][:title]
types = serialized[:included].map { |h| h[:type] }
assert_includes(types, 'active-model-serializer-default-include-test-posts')
assert_includes(types, 'active-model-serializer-default-include-test-authors')
end

test '#default_include merges with the render include for json_api adapter' do
serialized = serializable(@blog, serializer: BlogSerializer, adapter: :json_api, include: 'posts.category').as_json

assert_equal serialized[:included].size, 4
assert_equal 'The first post', serialized[:included].first[:attributes][:title]
types = serialized[:included].map { |h| h[:type] }
assert_includes(types, 'active-model-serializer-default-include-test-posts')
assert_includes(types, 'active-model-serializer-default-include-test-authors')
assert_includes(types, 'active-model-serializer-default-include-test-categories')
end

test '#default_include option include associations for attributes adapter' do
serialized = serializable(@blog, serializer: BlogSerializer, adapter: :attributes).as_json

assert_equal serialized[:posts].size, 2
assert_equal 'Blog Author', serialized[:posts].first[:author][:name]
end

test '#default_include merges with the render include for attributes adapter' do
serialized = serializable(@blog, serializer: BlogSerializer, adapter: :attributes, include: 'posts.category').as_json

assert_equal serialized[:posts].size, 2
assert_equal 'Blog Author', serialized[:posts].first[:author][:name]
assert_equal 'Food', serialized[:posts].first[:category][:name]
end
end
end
end