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

Support for Nested Relationships for the Json Adapter #1114

Closed
Closed
Show file tree
Hide file tree
Changes from 9 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
173 changes: 145 additions & 28 deletions lib/active_model/serializer/adapter/json.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,37 +7,13 @@ class Json < Adapter
def serializable_hash(options = nil)
options ||= {}
if serializer.respond_to?(:each)
@result = serializer.map { |s| FlattenJson.new(s).serializable_hash(options) }
@result = serialize_array_without_root(serializer, options)
else
@hash = {}
@result = resource_object_for(serializer)
@result = add_resource_relationships(@result, serializer)

@core = cache_check(serializer) do
serializer.attributes(options)
end

serializer.associations.each do |association|
serializer = association.serializer
opts = association.options

if serializer.respond_to?(:each)
array_serializer = serializer
@hash[association.key] = array_serializer.map do |item|
cache_check(item) do
item.attributes(opts)
end
end
else
@hash[association.key] =
if serializer && serializer.object
cache_check(serializer) do
serializer.attributes(options)
end
elsif opts[:virtual_value]
opts[:virtual_value]
end
end
end
@result = @core.merge @hash
@result
end

{ root => @result }
Expand All @@ -47,6 +23,147 @@ def fragment_cache(cached_hash, non_cached_hash)
Json::FragmentCache.new().fragment_cache(cached_hash, non_cached_hash)
end

private
Copy link
Member

Choose a reason for hiding this comment

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

👍


# iterate through the associations on the serializer,
# adding them to the parent as needed (as singular or plural)
#
# nested_associations is a list of symbols that governs what
# associations on the passed in seralizer to include
def add_resource_relationships(parent, serializer, nested_associations = [])
# have the include array normalized
nested_associations = include_array_to_hash(nested_associations)

included_associations = if nested_associations.present?
Copy link
Member

Choose a reason for hiding this comment

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

so nested_associations should be an normalized array with the nested objects you need.

Eg.
BlogSerializer with has_many :posts include: [:author]

so nested_associations will tell you that you need author but inside this if you check if it is included iinside serializer.associations with .select. But serializer.associations will have only posts.

Initially I thought this was to check if it's an already rendered relationship and avoid render it again. but it seems it is used here to loop the associations.

What I presume is the the code inside this if might never be executed unless you have something like (probably):

Eg.
BlogSerializer with has_many :posts include: [:author.posts]

then you will find the posts inside nested_associations as a member of serializer.associations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The if body actually decides what nested association to render.

Cause, lets say a post has a ton of associations, but you only want the author.
by the time you get to rendering the posts, [:author] will be passed in as nested_associations and the select filters for :author among all of a post's associations.

(The tests also cover this scenario)

serializer.associations.select{ |association|
# nested_associations is a hash of:
# key => nested association to include
nested_associations.has_key?(association.name)
}
Copy link
Member

Choose a reason for hiding this comment

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

Should probably be nested_associations.has_key?(association.name)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I dig it.

else
serializer.associations
end

included_associations.each do |association|
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 you can loop straight through serializer.associations here and delete the whole included_associations block before it because of what I mentioned

serializer = association.serializer
opts = association.options
key = association.key

# sanity check if the association has nesting data
has_nesting = nested_associations[key].present?
if has_nesting
include_options_from_parent = { include: nested_associations[key] }
opts = opts.merge(include_options_from_parent)
end

if serializer.respond_to?(:each)
parent[key] = add_relationships(serializer, opts)
else
parent[key] = add_relationship(serializer, opts)
end
end

parent
end

# add a singular relationship
# the options should always belong to the serializer
def add_relationship(serializer, options)
serialized_relationship = serialized_or_virtual_of(serializer, options)

nested_associations_to_include = options[:include]
if nested_associations_to_include.present?
serialized_relationship = add_resource_relationships(
serialized_relationship,
serializer,
nested_associations_to_include)
end

serialized_relationship
end

# add a many relationship
def add_relationships(serializer, options)
serialize_array(serializer, options) do |serialized_item, item_serializer|
nested_associations_to_include = options[:include]

if nested_associations_to_include.present?
serialized_item = add_resource_relationships(
serialized_item,
item_serializer,
nested_associations_to_include)
end

serialized_item
end
end


def serialize_array_without_root(serializer, options)
serializer.map { |s| FlattenJson.new(s).serializable_hash(options) }
end
Copy link
Member

Choose a reason for hiding this comment

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

I don't think the serializer should be calling the adapter... but I guess you didn't add that

Copy link
Member

Choose a reason for hiding this comment

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

the json_api adapter

serializer.each do |s|
result = self.class.new(s, @options.merge(fieldset: @fieldset)).serializable_hash(options)
does

          if serializer.respond_to?(:each)
            serializer.each do |s|
              result = self.class.new(s, @options.merge(fieldset: @fieldset)).serializable_hash(options)
              @hash[:data] << result[:data]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that doesn't look very nice to look at. :-\

aren't there json_api refactors in the prs?

I'm curious how that cleans things up.


# a virtual value is something that doesn't need a serializer,
# such as a ruby array, or any other raw value
def serialized_or_virtual_of(serializer, options)
if serializer && serializer.object
resource_object_for(serializer)
elsif options[:virtual_value]
options[:virtual_value]
end
end

def serialize_array(serializer, options)
serializer.map do |item|
serialized_item = resource_object_for(item)
serialized_item = yield(serialized_item, item) if block_given?
serialized_item
end
end

def resource_object_for(serializer)
Copy link
Member

Choose a reason for hiding this comment

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

Nice abstraction! 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks!

cache_check(serializer) do
serializer.attributes(serializer.options)
end
end

# converts the include hash to a standard format
# for constrained serialization recursion
#
# converts
# [:author, :comments => [:author]] to
# {:author => [], :comments => [:author]}
#
# and
# [:author, :comments => {:author => :bio}, :posts => [:comments]] to
# {:author => [], :comments => {:author => :bio}, :posts => [:comments]}
#
# The data passed in to this method should be an array where the last
# parameter is a hash
#
# the point of this method is to normalize the include
# options for the child relationships.
# if a sub inclusion is still an array after this method,
# it will get converted during the next iteration
def include_array_to_hash(include_array)
# still don't trust input
# but this also allows
# include: :author syntax
include_array = Array[*include_array].compact

result = {}

hashes = include_array.select{|a| a.is_a?(Hash)}
non_hashes = include_array - hashes

hashes += non_hashes.map{ |association_name| { association_name => [] } }
Copy link
Member

Choose a reason for hiding this comment

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

better to mutate hashes than generate a new object, I think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

how come?

Copy link
Member

Choose a reason for hiding this comment

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

this seems more like a function than a (private) property of the JSON Adapter.. What would you think about adding a

module ActiveModel::Serializer::Utils
  module_function

        def include_array_to_hash(include_array)
          # still don't trust input
          # but this also allows
          #  include: :author syntax
          include_array = Array[*include_array].compact

          result = {}

          hashes = include_array.select{|a| a.is_a?(Hash)}
          non_hashes = include_array - hashes

          hashes += non_hashes.map{ |association_name| { association_name => [] } }

          # now merge all the hashes
          hashes.each{|hash| result.merge!(hash) }

          result
        end
end

and then Utils.include_array_to_hash or mix it in

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like that. Calling it as a class metho would be my preference.


# now merge all the hashes
hashes.each{|hash| result.merge!(hash) }

result
end

end
end
end
Expand Down
203 changes: 203 additions & 0 deletions test/adapter/json/nested_relationships_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,203 @@
require 'test_helper'

NestedPostSerializer = Class.new(ActiveModel::Serializer) do
attributes :id, :title

has_many :comments, include: :author
end
Copy link
Member

Choose a reason for hiding this comment

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

I know this is the pattern in the poros.. but can we just do regular Ruby class declarations here, unless there's a good reason and we can comment that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd prefer normal class definitions.
I don't know why Class.new(...) is used though. :-\

I was just following existing patterns.


NestedCommentBelongsToSerializer = Class.new(ActiveModel::Serializer) do
attributes :id, :body

belongs_to :author, include: [:posts]
end

NestedAuthorSerializer = Class.new(ActiveModel::Serializer) do
attributes :id, :name

has_many :posts, include: [:comments]
end

ComplexNestedAuthorSerializer = Class.new(ActiveModel::Serializer) do
attributes :id, :name

# it would normally be silly to have this in production code, cause a post's
# author in this case is always going to be your root object
has_many :posts, include: [:author, comments: [:author]]
end
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to cover scenarios using multiple association declarations as well 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add a test for that real quick, just to be sure


module ActiveModel
class Serializer
class Adapter
class Json
class NestedRelationShipsTestTest < Minitest::Test
def setup
ActionController::Base.cache_store.clear
@author = Author.new(id: 1, name: 'Steve K.')

@post = Post.new(id: 42, title: 'New Post', body: 'Body')
@first_comment = Comment.new(id: 1, body: 'ZOMG A COMMENT')
@second_comment = Comment.new(id: 2, body: 'ZOMG ANOTHER COMMENT')

@post.comments = [@first_comment, @second_comment]
@post.author = @author

@first_comment.post = @post
@second_comment.post = @post

@blog = Blog.new(id: 1, name: "My Blog!!")
@post.blog = @blog
@author.posts = [@post]
end

def test_complex_nested_has_many
@first_comment.author = @author
@second_comment.author = @author

serializer = ComplexNestedAuthorSerializer.new(@author)
Copy link
Member

Choose a reason for hiding this comment

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

loving the name

adapter = ActiveModel::Serializer::Adapter::Json.new(serializer)
Copy link
Member

Choose a reason for hiding this comment

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

should be serializable_resource = ActiveModel::SerializableResource.new(@author, serializer: ComplexNestedAuthorSerializer, adapter: :json), then serializable_resource.serializable_hash, here and elsewhere, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't know you could do that. huh

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should I change it?
using serializableResource may introduce unneeded dependencies on other functionality

Copy link
Member

Choose a reason for hiding this comment

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

It's the way the controller does it.. so if that's what you're mimicing...

Copy link
Member

Choose a reason for hiding this comment

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

i.e. given a resource with serializer and adapter options, seralize it...

Copy link
Member

Choose a reason for hiding this comment

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

it also highlights the current inability to use adapters outside of the AMS namespace :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

personally, I feel that that what the controller does should be in separate tests. So that way if something breaks, if your tests are all separated by responsibility, it'll be easier to track down the cause.


expected = {
author: {
id: 1,
name: 'Steve K.',
posts: [
{
id: 42, title: 'New Post', body: 'Body',
author: {
id: 1,
name: 'Steve K.'
},
comments: [
{
id: 1, body: 'ZOMG A COMMENT',
author: {
id: 1,
name: 'Steve K.'
}
},
{
id: 2, body: 'ZOMG ANOTHER COMMENT',
author: {
id: 1,
name: 'Steve K.'
}
}
]
}
]
}
}

actual = adapter.serializable_hash

assert_equal(expected, actual)
end

def test_nested_has_many
serializer = NestedAuthorSerializer.new(@author)
adapter = ActiveModel::Serializer::Adapter::Json.new(serializer)

expected = {
author: {
id: 1,
name: 'Steve K.',
posts: [
{
id: 42, title: 'New Post', body: 'Body',
comments: [
{
id: 1, body: 'ZOMG A COMMENT'
},
{
id: 2, body: 'ZOMG ANOTHER COMMENT'
}
]
}
]
}
}

actual = adapter.serializable_hash

assert_equal(expected, actual)
end

def test_belongs_to_on_a_has_many
@first_comment.author = @author
@second_comment.author = @author

serializer = NestedPostSerializer.new(@post)
adapter = ActiveModel::Serializer::Adapter::Json.new(serializer)

expected = {
post: {
id: 42, title: 'New Post',
comments: [
{
id: 1, body: 'ZOMG A COMMENT',
author: {
id: 1,
name: 'Steve K.'
}
},
{
id: 2, body: 'ZOMG ANOTHER COMMENT',
author: {
id: 1,
name: 'Steve K.'
}
}
]
},
}

actual = adapter.serializable_hash

assert_equal(expected, actual)
end

def test_belongs_to_with_a_has_many
@author.roles = []
@author.bio = {}
@first_comment.author = @author
@second_comment.author = @author

serializer = NestedCommentBelongsToSerializer.new(@first_comment)
adapter = ActiveModel::Serializer::Adapter::Json.new(serializer)

expected = {
comment: {
id: 1, body: 'ZOMG A COMMENT',
author: {
id: 1,
name: 'Steve K.',
posts: [
{
id: 42, title: 'New Post', body: 'Body'
}
]
}
}
}

actual = adapter.serializable_hash

assert_equal(expected, actual)
end

def test_include_array_to_hash
serializer = NestedCommentBelongsToSerializer.new(@first_comment)
adapter = ActiveModel::Serializer::Adapter::Json.new(serializer)

expected = {author: [], comments: {author: :bio}, posts: [:comments]}
input = [:author, comments: {author: :bio}, posts: [:comments]]
actual = adapter.send(:include_array_to_hash, input)

assert_equal(expected, actual)
end
end
end
end
end
end