Add support for wildcards in nested includes #1158

beauby · 2015-09-16T06:48:55Z

Adds support for the following:

render json: posts, include: 'author, comments.*'
# includes the author, the comments, and all of the comments' related resources

render json: posts, include: [:author, comments: '*']
# same, with the array format

render json: posts, include: 'author, comments.**'
# includes the author, the comments, and all of the comments' related resources, and their related resources, etc.

Note that the wildcard (*) only expands one level, so you can chain them if you want more and keep control over the depth of the inclusion ('comments.*.*' for instance), or you can specify a double wildcard (**) if you want infinite expansion.

NullVoxPopuli · 2015-09-16T11:31:50Z

lib/active_model/serializer/adapter/json_api.rb

@@ -119,10 +119,26 @@ def relationships_for(serializer)
          Hash[serializer.associations.map { |association| [association.key, { data: relationship_value_for(association.serializer, association.options) }] }]
        end

+        def expand_includes(includes, associations)


can you write some yardoc about what this method does, and some example inputs and outputs?

beauby · 2015-09-16T16:09:52Z

Now also works with (Flatten)Json adapters.

NullVoxPopuli · 2015-09-16T16:23:09Z

lib/active_model/serializer.rb

+    # and corresponding inclusion hashes.
+    # @param [Hash] includes
+    # @return [Array] an array of pairs [association, include_hash] for matching associations
+    def expand_includes(includes)


Should this format be different from the other format?
I'd prefer it to be the same, hash of hashes

This one has a different purpose. I'm not happy with the name of this method, because what it really does is lookup each included association and returns the corresponding Association and include hash. It just happens to expand stuff because at this stage we can't avoid it anymore.

Having this function just return a hash would force an other lookup of the associations down the road.

beauby · 2015-09-17T09:35:17Z

lib/active_model/serializer/adapter/json.rb

+                  association.options[:virtual_value]
+                elsif association.serializer && association.serializer.object
+                  FlattenJson.new(association.serializer,
+                                  association.options.merge(include: assoc_includes))


This line gives a rubocop warning about aligning method parameters that span multiple lines, although I believe the indentation is right here.

Have the first param on the next line, indented two spaces from the start of the F?

According to https://github.com/bbatsov/ruby-style-guide#no-double-indent it should be ok like that.

could be a bug in rubocop. I don't think it validates itself against the style guide and diverges at times

but there's definitley too much going on in that block

beauby · 2015-09-21T00:39:50Z

lib/active_model/serializer/adapter/attributes.rb

@@ -2,6 +2,11 @@ module ActiveModel
  class Serializer
    module Adapter
      class Attributes < Base
+        def initialize(serializer, options = {})
+          super
+          @include_tree = ActiveModel::Serializer::IncludeTree.from_include_args(options[:include] || '*')


By default, include all the toplevel specified association (current behavior).

should the default value be move to the method?

No, because we want the flexibility to have different default values per adapter. For instance, the JsonApi adapter defaults to none (since the relationship objects themselves will be serialized anyways, we don't want to enforce the related resources to be included in the answer unless specified by the user).

Does this need to be in the initializer? Does it need to be eager evaluated? Also, you could

def initialize(*) super # use whatever the super method did

which is more resistant to changes in method signature or whatever

NullVoxPopuli · 2015-09-21T01:14:47Z

lib/active_model/serializer/include_tree.rb

+        #
+        # @param [String] included
+        # @return [IncludeTree]
+        #


yay documentation!!!

but I think @return is supposed to be the last line of the comment?

Not sure, there are many places in the current code base where the format is as above. I don't mind changing it, once we make a consistent choice for the future.

Long class << self blocks are hard to read. Better to just use def self.method_name.

I only did it in Adapter because I was class << Adapter which is easier to read than def Adapter.whatever and a little less surprising, but also I was intending to get rid of it eventually.

I tend to agree, although it's a pattern that is wildly used within rails. What do you think? I don't mind changing it.

I'd prefer def self.method_name unless you feel it's clearer or better otherwise

Looks good to me, updating immediately.

Also, some of these styles will need to be autocorrected if we merge #1183

I don't think Helper is a good name, but I wanted to just get the idea across :)

Yeah, I thought of Parsing instead, what do you think?

or Transformation.. but since it's internal so far, I'm not too concerned

bf4 · 2015-09-21T01:35:06Z

I think this PR would benefit from including documentation of usage as well as caveats. I see this feature as a major bug report attractor, so would be great to just point to docs which describe what it can't be guaranteed to do well, or performantly.

beauby · 2015-09-21T01:48:11Z

My intent was to build it first for internal usage (the wildcards themselves make the nested serializers much easier to write), and document it in a subsequent PR.

bf4 · 2015-09-21T02:01:05Z

@beauby Ok with me if you're doing it in steps like that. I haven't had the issue this PR is solving, so I'm not a 'domain' expert on what's best for AMS in this regard

beauby · 2015-09-21T02:29:20Z

lib/active_model/serializer.rb

@@ -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'


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

bf4 · 2015-09-21T03:50:05Z

lib/active_model/serializer/include_tree.rb

+        when @hash.key?(:**)
+          self.class.new(:** => {})
+        else
+          nil


@beauby You're saying Rubocop didn't like this?

I am. https://travis-ci.org/rails-api/active_model_serializers/jobs/81323442

Add support for wildcards in nested includes

joaomdmoura · 2015-10-03T00:04:27Z

Nice! 👍

beauby force-pushed the includes-wildcard branch 3 times, most recently from f0bf341 to 883a684 Compare September 16, 2015 07:05

This was referenced Sep 16, 2015

Improve jsonapi include #1159

Closed

Add support for nested serializers. #1157

Closed

NullVoxPopuli reviewed Sep 16, 2015
View reviewed changes

NullVoxPopuli added the V: 0.10.x label Sep 16, 2015

beauby mentioned this pull request Sep 16, 2015

[RFC]Thoughts on an explicit way to define nested relationships? #1113

Closed

beauby changed the title ~~Add support for wildcards in nested includes for JsonApi~~ Add support for wildcards in nested includes Sep 16, 2015

beauby force-pushed the includes-wildcard branch from 5d62e82 to 379c800 Compare September 16, 2015 16:12

NullVoxPopuli reviewed Sep 16, 2015
View reviewed changes

beauby added the C: Feature label Sep 16, 2015

beauby reviewed Sep 17, 2015
View reviewed changes

beauby force-pushed the includes-wildcard branch 9 times, most recently from 52e5433 to 0de720d Compare September 21, 2015 00:27

beauby reviewed Sep 21, 2015
View reviewed changes

NullVoxPopuli reviewed Sep 21, 2015
View reviewed changes

beauby force-pushed the includes-wildcard branch from c396b0f to b6013c7 Compare September 21, 2015 02:28

beauby reviewed Sep 21, 2015
View reviewed changes

beauby mentioned this pull request Sep 21, 2015

Options not passed from Json to Attributes adapters #1184

Closed

beauby force-pushed the includes-wildcard branch from f9ee95a to 53d780b Compare September 21, 2015 03:07

bf4 reviewed Sep 21, 2015
View reviewed changes

Add support for wildcard includes + improve perfs on JsonApi includes.

ac06013

beauby force-pushed the includes-wildcard branch from 53d780b to ac06013 Compare September 21, 2015 03:58

NullVoxPopuli added a commit that referenced this pull request Sep 21, 2015

Merge pull request #1158 from beauby/includes-wildcard

aaa60bf

Add support for wildcards in nested includes

NullVoxPopuli merged commit aaa60bf into rails-api:master Sep 21, 2015

NullVoxPopuli mentioned this pull request Sep 21, 2015

Create NestedJson adapter. #1073

Closed

lautis mentioned this pull request Sep 21, 2015

Attribute filtering for 0.10.x #1141

Closed

This was referenced Sep 22, 2015

Nested serializers/attributes #1190

Closed

Load associations to one level while conditionally sideloading associations in Active model serializers #927

Closed

Nested Associations #1077

Closed

JSON adapter does not handle nested associations #835

Closed

bf4 deleted the includes-wildcard branch October 9, 2015 06:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for wildcards in nested includes #1158

Add support for wildcards in nested includes #1158

beauby commented Sep 16, 2015

NullVoxPopuli Sep 16, 2015

beauby Sep 16, 2015

beauby commented Sep 16, 2015

NullVoxPopuli Sep 16, 2015

beauby Sep 16, 2015

beauby Sep 16, 2015

NullVoxPopuli Sep 16, 2015

beauby Sep 17, 2015

NullVoxPopuli Sep 18, 2015

beauby Sep 18, 2015

bf4 Sep 18, 2015

bf4 Sep 18, 2015

beauby Sep 21, 2015

NullVoxPopuli Sep 21, 2015

beauby Sep 21, 2015

bf4 Sep 21, 2015

NullVoxPopuli Sep 21, 2015

beauby Sep 21, 2015

bf4 Sep 21, 2015

beauby Sep 21, 2015

bf4 Sep 21, 2015

beauby Sep 21, 2015

bf4 Sep 21, 2015

bf4 Sep 21, 2015

beauby Sep 21, 2015

bf4 Sep 21, 2015

bf4 commented Sep 21, 2015

beauby commented Sep 21, 2015

bf4 commented Sep 21, 2015

beauby Sep 21, 2015

bf4 Sep 21, 2015

beauby Sep 21, 2015

joaomdmoura commented Oct 3, 2015

Add support for wildcards in nested includes #1158

Add support for wildcards in nested includes #1158

Conversation

beauby commented Sep 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beauby commented Sep 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bf4 commented Sep 21, 2015

beauby commented Sep 21, 2015

bf4 commented Sep 21, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joaomdmoura commented Oct 3, 2015