Skip to content

Add support for wildcards in nested includes #1158

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

Merged
merged 1 commit into from
Sep 21, 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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ Features:
* adds FlattenJSON as default adapter [@joaomdmoura]
* adds support for `pagination links` at top level of JsonApi adapter [@bacarini]
* adds extended format for `include` option to JsonApi adapter [@beauby]
* adds support for wildcards in `include` option [@beauby]

Fixes:

Expand Down
2 changes: 1 addition & 1 deletion lib/active_model/serializer.rb
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
require 'thread_safe'
require 'active_model/serializer/adapter'
require 'active_model/serializer/array_serializer'
require 'active_model/serializer/include_tree'
require 'active_model/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.

It has to be before associations, because we use it in there.

require 'active_model/serializer/configuration'
require 'active_model/serializer/fieldset'
require 'active_model/serializer/lint'
require 'active_model/serializer/utils'

module ActiveModel
class Serializer
Expand Down
7 changes: 6 additions & 1 deletion lib/active_model/serializer/adapter/attributes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,11 @@ module ActiveModel
class Serializer
module Adapter
class Attributes < Base
def initialize(serializer, options = {})
super
@include_tree = IncludeTree.from_include_args(options[:include] || '*')
Copy link
Contributor

Choose a reason for hiding this comment

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

should just options[:include] be passed, and then in the method, do include || '*'?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you be more specific? If you're referring to doing @include_tree = IncludeTree.from_include_args(options[:include]) || '*', then it wouldn't work as 1. #from_include_args always returns an empty IncludeTree for "shady" inputs, and 2. even if that was not the case, we would still have to parse the '*'.

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe like this:

IncludeTree.from_include_args(options[:include])

def from_include_args(included)
  new(include_args_to_hash(included || '*'))
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doesn't work for the reasons specified somewhere else: we need the flexibility of different fallbacks depending on the adapter.

Copy link
Contributor

Choose a reason for hiding this comment

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

gotchya. What if the class-level method to get the include associations tree per-adapter? adapters could just override that method if they need to, and we could have a generic one in base?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure I understand. Could you be more specific?

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe something like this?

def self.included_associations(options)
  IncludeTree.from_include_args(options[:include] || '*')
end

and then @include_tree = included_associations(options) ?

idk

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure it adds much value at this stage, but I am not opposed to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's keep it like that for now, and refactor it later if it makes stuff easier?

end

def serializable_hash(options = nil)
options ||= {}
if serializer.respond_to?(:each)
Expand All @@ -13,7 +18,7 @@ def serializable_hash(options = nil)
serializer.attributes(options)
end

serializer.associations.each do |association|
serializer.associations(@include_tree).each do |association|
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 this

Copy link
Member

Choose a reason for hiding this comment

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

sorta railsy

serializer = association.serializer
association_options = association.options

Expand Down
52 changes: 27 additions & 25 deletions lib/active_model/serializer/adapter/json_api.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@ class JsonApi < Base

def initialize(serializer, options = {})
super
@included = ActiveModel::Serializer::Utils.include_args_to_hash(instance_options[:include])
@include_tree = IncludeTree.from_include_args(options[:include])

fields = options.delete(:fields)
if fields
@fieldset = ActiveModel::Serializer::Fieldset.new(fields, serializer.json_key)
Expand All @@ -19,10 +20,11 @@ def initialize(serializer, options = {})

def serializable_hash(options = nil)
options ||= {}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not required for this PR, but passing the serializer reference along is useless here as it is available via an attr_reader.

Copy link
Contributor

Choose a reason for hiding this comment

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

because of the new associations @include stuff :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean?

Copy link
Member

Choose a reason for hiding this comment

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

I'd kind of like to simplify defining def serializable_hash in subclasses so that we can either def serializable_hash(*); super; whatever or def _serializable_hash(options) a method called by serializable_hash, or alternatively, even just use read_attribute_for_serialization

Copy link
Contributor

Choose a reason for hiding this comment

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

scratch that, I thought I was looking at a different adapter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bf4: Not sure I understand.

Copy link
Member

Choose a reason for hiding this comment

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

Basically just avoid the

         def serializable_hash(options = nil)
           options ||= {}

boilerplate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I see. I'm all up for that, although I don't think it falls within the scope of this PR. Moreover, I think we should have serializable_hash defined in Base, and have serializers override serializable_hash_for_(collection|single_resource).

Copy link
Member

Choose a reason for hiding this comment

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

pretty much the same as what I'm thinking. Yes, outside of scope

if serializer.respond_to?(:each)
serializable_hash_for_collection(serializer, options)
serializable_hash_for_collection(options)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not required for this PR, but passing the serializer reference along is useless here as it is available via an attr_reader.

else
serializable_hash_for_single_resource(serializer, options)
serializable_hash_for_single_resource(options)
end
end

Expand All @@ -34,10 +36,10 @@ def fragment_cache(cached_hash, non_cached_hash)
private

ActiveModel.silence_warnings do
attr_reader :included, :fieldset
attr_reader :fieldset
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not required for this PR, but passing the serializer reference along is useless here as it is available via an attr_reader.


def serializable_hash_for_collection(serializer, options)
def serializable_hash_for_collection(options)
hash = { data: [] }
serializer.each do |s|
result = self.class.new(s, instance_options.merge(fieldset: fieldset)).serializable_hash(options)
Expand All @@ -57,10 +59,10 @@ def serializable_hash_for_collection(serializer, options)
hash
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not required for this PR, but passing the serializer reference along is useless here as it is available via an attr_reader.


def serializable_hash_for_single_resource(serializer, options)
def serializable_hash_for_single_resource(options)
primary_data = primary_data_for(serializer, options)
relationships = relationships_for(serializer)
included = included_for(serializer)
included = included_resources(@include_tree)
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 know there's a trend towards avoiding instance variables, but I think they make the situation really clear, compared to private accessors.

Copy link
Member

Choose a reason for hiding this comment

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

my concern with ivars is that they're more buggy. If I type @includ_tree I get some method called on nil error which I have to track down. If I call includ_tree, it blows up at load. So, I tend not to make them outside of the initializers or getters and setters. I get that there's probably some performance hit

Copy link
Contributor

Choose a reason for hiding this comment

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

that is true - 👍 @bf4

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My concern is not that much towards performance than clarity: it's easy to define a local variable with the same name as a getter/setter. Plus @var makes it really clear where to look for when something is wrong with @var. I guess it's a tradeoff.

hash = { data: primary_data }
hash[:data][:relationships] = relationships if relationships.any?
hash[:included] = included if included.any?
Expand Down Expand Up @@ -123,37 +125,37 @@ def relationship_value_for(serializer, options = {})
end

def relationships_for(serializer)
Hash[serializer.associations.map { |association| [association.key, { data: relationship_value_for(association.serializer, association.options) }] }]
serializer.associations.each_with_object({}) do |association, hash|
hash[association.key] = { data: relationship_value_for(association.serializer, association.options) }
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not required for this PR, but it feels nicer this way.

end

def included_for(serializer)
included.flat_map { |inc|
association = serializer.associations.find { |assoc| assoc.key == inc.first }
_included_for(association.serializer, inc.second) if association
}.uniq
def included_resources(include_tree)
included = []

serializer.associations(include_tree).each do |association|
add_included_resources_for(association.serializer, include_tree[association.key], included)
end

included
end

def _included_for(serializer, includes)
def add_included_resources_for(serializer, include_tree, included)
if serializer.respond_to?(:each)
serializer.flat_map { |s| _included_for(s, includes) }.uniq
serializer.each { |s| add_included_resources_for(s, include_tree, included) }
else
return [] unless serializer && serializer.object
return unless serializer && serializer.object

primary_data = primary_data_for(serializer, instance_options)
relationships = relationships_for(serializer)
primary_data[:relationships] = relationships if relationships.any?

included = [primary_data]
return if included.include?(primary_data)
included.push(primary_data)

includes.each do |inc|
association = serializer.associations.find { |assoc| assoc.key == inc.first }
if association
included.concat(_included_for(association.serializer, inc.second))
included.uniq!
end
serializer.associations(include_tree).each do |association|
add_included_resources_for(association.serializer, include_tree[association.key], included)
end

included
end
end

Expand Down
6 changes: 5 additions & 1 deletion lib/active_model/serializer/associations.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ class Serializer
module Associations
extend ActiveSupport::Concern

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

included do |base|
class << base
attr_accessor :_reflections
Expand Down Expand Up @@ -82,13 +84,15 @@ def associate(reflection)
end
end

# @param [IncludeTree] include_tree (defaults to all associations when not provided)
# @return [Enumerator<Association>]
#
def associations
def associations(include_tree = DEFAULT_INCLUDE_TREE)
return unless object

Enumerator.new do |y|
self.class._reflections.each do |reflection|
next unless include_tree.key?(reflection.name)
y.yield reflection.build_association(self, instance_options)
end
end
Expand Down
75 changes: 75 additions & 0 deletions lib/active_model/serializer/include_tree.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
module ActiveModel
class Serializer
class IncludeTree
module Parsing
Copy link
Member

Choose a reason for hiding this comment

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

This is the same as ActiveModel::Serializer::Utils was, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.

module_function

def include_string_to_hash(included)
included.delete(' ').split(',').reduce({}) do |hash, path|
include_tree = path.split('.').reverse_each.reduce({}) { |a, e| { e.to_sym => a } }
hash.deep_merge!(include_tree)
end
end

def include_args_to_hash(included)
case included
when Symbol
{ included => {} }
when Hash
included.each_with_object({}) { |(key, value), hash|
Copy link
Contributor

Choose a reason for hiding this comment

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

do end for multiline block?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I'm not too sure since it's not technically a multiline block (there's only one line in the block), and this format is present in the codebase already. I don't mind changing it, but let's make it a lasting decision then.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, idk. according to the styleguide, this should all be on one line -- but it's so long. I wonder if this should be extracted to a method, like you did with the string

Copy link
Member

Choose a reason for hiding this comment

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

Two private class methods all taking the included argument? Seems a latent object to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

included are different things in those contexts.

Copy link
Member

Choose a reason for hiding this comment

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

naming?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They could basically be renamed include_string and include_object (the latter could be almost anything). Would that seem better to you?

Copy link
Member

Choose a reason for hiding this comment

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

Sure. Mind you, I'm kind of tired, so anything that makes me think less sounds good :)

hash[key] = include_args_to_hash(value)
}
when Array
included.reduce({}) { |a, e| a.merge!(include_args_to_hash(e)) }
when String
include_string_to_hash(included)
else
{}
end
end
end

# Builds an IncludeTree from a comma separated list of dot separated paths (JSON API format).
# @example `'posts.author, posts.comments.upvotes, posts.comments.author'`
#
# @param [String] included
# @return [IncludeTree]
#
def self.from_string(included)
new(Parsing.include_string_to_hash(included))
end

# Translates the arguments passed to the include option into an IncludeTree.
# The format can be either a String (see #from_string), an Array of Symbols and Hashes, or a mix of both.
# @example `posts: [:author, comments: [:author, :upvotes]]`
#
# @param [Symbol, Hash, Array, String] included
# @return [IncludeTree]
#
def self.from_include_args(included)
new(Parsing.include_args_to_hash(included))
end

# @param [Hash] hash
def initialize(hash = {})
@hash = hash
end

def key?(key)
@hash.key?(key) || @hash.key?(:*) || @hash.key?(:**)
end

def [](key)
# TODO(beauby): Adopt a lazy caching strategy for generating subtrees.
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

Choose a reason for hiding this comment

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

does this mean the PR merging should wait on this being done?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it does not. Just that it's something we will probably want to do in a subsequent PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

cool

case
when @hash.key?(key)
self.class.new(@hash[key])
when @hash.key?(:*)
self.class.new(@hash[:*])
when @hash.key?(:**)
self.class.new(:** => {})
Copy link
Member

Choose a reason for hiding this comment

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

A case missing an else is a bug waiting to happen. In part, it doesn't tell me whether you intend a no-op or hadn't thought of what the behavior should be.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to the Ruby Style Guide, one should avoid redundant else-clauses.

Copy link
Member

Choose a reason for hiding this comment

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

Link? I don't see it, nor do I think it's a style issue

Copy link
Contributor

Choose a reason for hiding this comment

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

is the intent to return nil for the else case?

if so, a method yardoc would be great :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My mistake, it was just my linter. Will update immediately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

end
end
end
end
end
35 changes: 0 additions & 35 deletions lib/active_model/serializer/utils.rb

This file was deleted.

6 changes: 3 additions & 3 deletions test/action_controller/json_api/linked_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,9 @@ def render_resource_with_nested_include
render json: @post, include: [comments: [:author]], adapter: :json_api
end

def render_resource_with_nested_has_many_include
def render_resource_with_nested_has_many_include_wildcard
setup_post
render json: @post, include: 'author.roles', adapter: :json_api
render json: @post, include: 'author.*', adapter: :json_api
end
Copy link
Contributor

Choose a reason for hiding this comment

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

should there be an additional test instead of changing an existing one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There should be many more tests, indeed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I don't believe there should be more integration tests for this PR. We should just thoroughly test the expand_includes method.

Copy link
Member

Choose a reason for hiding this comment

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

We should just thoroughly test the expand_includes method.

@beauby Yes, moar unit tests

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 method does not exist anymore.


def render_resource_with_missing_nested_has_many_include
Expand Down Expand Up @@ -96,7 +96,7 @@ def test_render_resource_with_include
end

def test_render_resource_with_nested_has_many_include
get :render_resource_with_nested_has_many_include
get :render_resource_with_nested_has_many_include_wildcard
response = JSON.parse(@response.body)
expected_linked = [
{
Expand Down
26 changes: 26 additions & 0 deletions test/include_tree/from_include_args_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
require 'test_helper'

module ActiveModel
class Serializer
class IncludeTree
class FromStringTest < Minitest::Test
def test_simple_array
input = [:comments, :author]
actual = ActiveModel::Serializer::IncludeTree.from_include_args(input)
assert(actual.key?(:author))
assert(actual.key?(:comments))
end

def test_nested_array
input = [:comments, posts: [:author, comments: [:author]]]
actual = ActiveModel::Serializer::IncludeTree.from_include_args(input)
assert(actual.key?(:posts))
assert(actual[:posts].key?(:author))
assert(actual[:posts].key?(:comments))
assert(actual[:posts][:comments].key?(:author))
assert(actual.key?(:comments))
end
end
end
end
end
Loading