Include option for associations in serializers

[oyeanuj]

I am migrating from 0.9.3 to 0.10.0-rc3 to take advantage of json-api. Based on the current docs, it seems if I want the association's data as part of the response, I will have to specify it as part of 'include' option when calling render in the controller.

Is there a way to specify this in the serializer instead of the controller, so that I can specify it once and everytime I render that serializer, those associations are included in the json-api by default (instead of having to specify it every time I render that object)? That seemed to be the default behavior in the 0.9.x

TLDR: Instead of

# posts_controller.rb
render @post, include: ['author', 'metrics', 'media']

something like this:

# post_serializer.rb

has_many :metrics #or included by default which would be ideal
has_many :media, only: [title]
has_many :publishers, except: [publisher_bio]

# or even
has_one :author, include: true 

Maybe it is already there and I missed it. Either way, I would love some clarity.

Thanks!

[beauby]

Not at the moment, but there is discussion about it.

[oyeanuj]

Thanks for the quick response!

Fwiw, I think having them included by default would make the upgrade path from 0.8 and 0.9 to 0.10 much easier and more backwards compatible (and not needing to include associations every time or refactor code to prevent repetition).

[iMacTia]

@oyeanuj, PR #1127 does exactly what you're asking, but unfortunately have been merged the 21st of September, only 5 days after 0.10.0-rc3 was released.
@joaomdmoura, any plan for the rc4? this feature have been requested by many and rc3 have been out for almost 2 months now.

[oyeanuj]

@iMacTia Thats really good to know, thank you for that. Couple of questions with your PR:

1. Without the include, does it only get the type and id? And include specific fields if we want more of that object? Ideally to maintain backward compatibility, there would be an include: false option which only then shows the id & type but by default associations would be included.
2. Does your PR also support except and only options on associations too for backwards compatibility?

@joaomdmoura If I can add to the rc4 question :) - when is the 0.10 final release roughly aimed for?

Thanks again!

[iMacTia]

@oyeanuj unfortunately I can't take the merit for that PR because is all @NullVoxPopuli work :)
So it's quite hard for me to correctly answer your question, also because I've never used the json-api adapter so far, but only the standard json one.
I'm simply really interested as much as you are on getting this new features out od the door ;) So sometimes I open the project page to see what's the status.

[beauby]

@oyeanuj

1. Have a look at the JSON API spec. The idea is that it differentiates between "resources" (actual objects) and "resource identifiers" (only type and id, which are basically references to other resources). When a resource is referenced as a relationship for an other resource, only its resource identifier appears. Using include, you can have the actual resource in the included part of the JSON document.
2. No except / only for now, although it is being discussed.

[oyeanuj]

@beauby Thanks for the context, I was missing that.

In that case, it would be really helpful to have include',except,only` options in the serializer when specifying associations. That would make the upgrading process much easier wherein folks don't have to rework everyplace where they are currently using those options in the 0.8 and 0.9 versions.

Thanks again for your work on this library!

[beauby]

I'll try to issue a PR that adds support for only/except on associations tonight, we'll see whether it gets traction!

[oyeanuj]

@beauby thank you, appreciate it!

[bf4]

@beauby

I'll try to issue a PR that adds support for only/except on associations tonight, we'll see whether it gets traction!

For JSON API, there shouldn't be a need for only exclude, since it's always opt-in. Otherwise, it would be an argument to the include tree thingy, right?

[oyeanuj]

@bf4 Having the only/except also is allowing for major backwards compatibility for those upgrading from 0.8.x and 0.9.x. When dealing with associations in serializers, only and except turn out to be invaluable to customize API responses.

[bf4]

You're right. I keep mixing up internal vs external options. Internal like here would work well with serilization scopes and requested attributes

B mobile phone

On Dec 8, 2015, at 10:52 PM, oyeanuj [email protected] wrote:

@bf4 Having the only/except also is allowing for major backwards compatibility for those upgrading from 0.8.x and 0.9.x. When dealing with associations in serializers, only and except turn out to be invaluable to customize API responses.

—
Reply to this email directly or view it on GitHub.

[beauby]

@bf4 There will be some discussion about priorities of the options, but my current thought is "if an explicit include option is provided to the render call, disregard the serializer-level options".

[bf4]

Agreed

B mobile phone

On Dec 9, 2015, at 6:16 AM, Lucas Hosseini [email protected] wrote:

@bf4 There will be some discussion about priorities of the options, but my current thought is "if an explicit include option is provided to the render call, disregard the serializer-level options".

—
Reply to this email directly or view it on GitHub.

[oyeanuj]

@beauby Did this ever make it in to 0.10 and documented?

[tommiller1]

Is anyone working on adding the 'include: true' option to associations? Would be nice to couple 'include' with 'if' so we can move inclusion logic to the serializers.

Also 'include' sounds ambiguous as all attributes/associations are included by default (unless they are nested). Perhaps 'always_include' or something.

[iMacTia]

I would like to 👍 +1 on this one and to trigger the "Team Discussion" needed to progress on this :)
Just finished chatting about it with @bf4 and @NullVoxPopuli in #1828, this would be a really nice and useful addition to those sticking with the good old standard JSON adapter.

I understand the concerns around this feature, as @bf4 already pointed out:

Bear in mind, that AMS is oriented to using the semantics of jsonapi.org/format/ which doesn't map well to your implementation. The AMS way to do that would be to use default params in your render call, rather than patch the serializer. i.e. something like

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

That would have the same effect, assuming you're using JSON API, but not require the serializer only know how to map attributes and relationships, but not how you want to render them.

And @NullVoxPopuli added:

since JSON API is the most full-featured, we're based all the adapter functionality off of that. We also want to ensure the apis are the same in all adapters, (which we currently don't have) -- so this feature request may be implemented post 1.0

[...] Having different options between the adapters is confusing. All adapters should have the same options. Otherwise things are too coupled to specific implementations.

So here we are, do you think we can have a chat about it and try to find an implementation that would suite everyone? I made a PR in the past to add this capability:
master...iMacTia:master
But it only works with the JSON adapter, not for the JSON API one.

==== Discussion time! ====

First of all, I would like to reply to @bf4 because this looks to me like the most important decision in this matter. I had a quick read about the jsonapi semantics and I think this is the paragraph concerning our case: http://jsonapi.org/format/#fetching-includes
As you can read:

An endpoint MAY return resources related to the primary data by default.
An endpoint MAY also support an include request parameter to allow the client to customize which related resources should be returned.

My interpretation is that having default nested resources and don't allowing a customisation through the include parameter is legit according to the document. That said, if I have the same serialiser used in many controllers or embedded in many other resources, it could be hard for me to copy-and-paste the include option in all controller actions.

The solution to this (edge?) case is to allow people to specify at serialiser level which associations will ALWAYS be included, regardless of the context.

I really think we should all agree on this before going forward, would love to hear your thought on this :)

[NullVoxPopuli]

@iMacTia thanks for posting your code!

Some questions I have:

• if a has_many is specified as included: true, what happens when someone uses the render include option? is he serializer option ignored? is it ALWAYS included? (I think always is the most intuitive -- so the only way to not include that has_many is to whitelist fields other than that has_many)
• should the api for this be included: true (defaulting to included: false, implicitly, of course)

[iMacTia]

Very interesting questions, I asked myself the same and here is my personal opinion:

1. The association include option will not substitute the render one, they simple fit two different scenarios. The former intrinsically means "when using this serialiser, you should always include this association", whether the latter means "in this specific case, include these associations in the rendering". So it wouldn't really make sense to specify the association include and then remove it in some cases, that would mean that the render include should be used instead. That said, I can already see people asking for an option to do it in order to manage edge cases, but I personally see it as an addition that can be done later.
2. I have been thinking about it, and I personally see other solutions for it. The more we discuss about it, the more I can see a similarity with ActiveRecord::Base#default_scope. So another possibility would be to add something like default_includes: [:metrics, :author] directly as a serialiser class method. This would follow exactly the same api as the render include.
   Again, foreseeing users requests, mimicking ActiveRecord::Base#default_scope we could always have an unscoped option to override the default. But as I stated in the previous point, I don't see it as an initial requirement.

Here is an example for point 2 (including a 2-level nesting):

# post_serializer.rb
default_includes :media, :author

has_many :media, only: [:title]
has_one :author

# author_serializer.rb
default_includes :website

has_one :website, only: [:url]

# posts_controller.rb
render @post

would be the equivalent of

# post_serializer.rb
has_many :media # not sure if `only: [:title]` should go here as well
has_one :author

# author_serializer.rb
has_one :website # not sure if `only: [:url]` should go here as well

# posts_controller.rb
render json: @post, include: [:media, author: [:website]],
    fields: { media: [:title], author: { website: [:url] } }

[oyeanuj]

So happy to see this issue come alive again! Thanks @iMacTia @NullVoxPopuli :)

Couple of questions which I'd like clarified -

1. It seems like the options are being discussed only for JSON adapter and not the JSON-API adapter. When I originally created the issue, I was hoping to migrate the JSON-API adapter as well. Reading through the JSON-API spec, it doesn't seem like the JSON-API adapter can't have these changes. The only thing that I see is that with by default, JSON-API, renders references to the included entities?

The changes discussed seem to be really a convenience for the purposes of coding what the defaults should be for different objects - one that shouldn't be affected by the adapter itself. Sorry, if I am misunderstanding some part of the conversation here - would love to understand what I am missing.

1. When we talk about include, do we also implicitly include only and except in that as those just seem to be just convenient aliases semantically?
2. @iMacTia @NullVoxPopuli Curious why the AMS 0.9x convention and syntax of specifying associations doesn't work as-is in AMS 0.10.x?

[NullVoxPopuli]

@iMacTia,

So another possibility would be to add something like default_includes:

I like this idea, maybe more than having an includes option on each association definition.
good thinkin'!

@oyeanuj,
It is my personal goal to make sure all options are available in all adapters. :-)

something to keep in mind:
include always references relationships.
@bf4 has stated that only/except can be confusing, especially when used together.
also, I think only / except are for attributes within a particular serializer.
So, good thing to bring up, @oyeanuj , we def don't want to add any confusion with this -- which is why I like @iMacTia 's idea of default_includes

0.8, 0.9, and 0.10 have all been rewrites :-(

we're getting there with 0.10. progress is just slow. :-(

[iMacTia]

@oyeanuj I'm sorry for not being clear enough on this point, but since the render include option works with all adapters (it does, right?), the new default_includes would be adapter-agnostic as well.

Regarding the only/except, I have to confess I don't really know exactly how they currently work (that's why I added those comments in my previous example), but the most logical output for me would be to always apply them when including associations.

[tommiller1]

+1 for default_includes, it could be used for conditionally including associations too. WIth the include: true option, the next feature request will be include: :is_current_user? etc. It is also intuitive that default_includes will be merged with the include param on the render call.

[iMacTia]

@tommiller1 not sure what do you mean by

it could be used for conditionally including associations too

could you provide an example :)?

@NullVoxPopuli it looks like this default_includes makes sense to others, I'll fork (again) the repo and start working on it

[tommiller1]

If you look at the ':if' keyword in ams, it supports blocks and lambdas to allow conditions. Similarly a ':default' keyword would also be expected to support all that. Anyway nevermind, the 'default_includes' is probably the most elegant and unobtrusive way of adding this functionality. Thanks for working on it, hope the creators will accept your pr.

[iMacTia]

Thanks! Will do my best to get something done to have this feature in place as soon as possible, with the help of the AMS team. After that, it could be the starting point for additional sub-features like the one you described :)

[iMacTia]

Ok guys, quick update.
After spending some time digging into the code this morning, I now have a better idea of how the architecture works.
The easiest solution I can see, is a little more restrictive than what I initially planned, but I also think it would still fit most cases.

The idea is to modify ActiveModel::Serializer::Associations#associations
This method will be called at some point by any adapter to retrieve the list of associations for a given serialiser instance.
The parameter include_directive is a JSONAPI::IncludeDirective object, based on the include option of the adapter.

It should be enough to merge the include_directive parameter with the serialiser class one (easily accessible from here through self.class.default_inclides).
Potentially one line of code to achieve what we wanted.

My concerns:

1. What should be the value for default_includes? Is it just an Array of symbols (association names) or an Hash to specify multi-level inclusion? Initial idea was to have the same format as the render include option, but I know realise that could not be possible.
2. What's the best way to merge include_directive param with default_includes? Should we work on a copy of include_directive or can we mutate the original one?

As always, let me know your thoughts :)

[NullVoxPopuli]

I think the value of default_includes should be nil

I think the format of how it's specified should be the same as the include param, then you could do:

include_from_options = JSONAPI::IncludeDirective(options[:include])
default_includes = JSONAPI::IncludeDirective(serializer.default_includes)
include_directive = default_includes.deep_merge(include_from_options)

[iMacTia]

@NullVoxPopuli: thanks, I've been thinking the same and that would be great!
But unfortunately I'm concerned by a couple of issues here:

1. Deep merge is not a method of JSONAPI::IncludeDirective (we can always implement it :D)
2. Are we sure that doing this, nested includes will be correctly passed to the associations serialisers?

[NullVoxPopuli]

@iMacTia you can get the hashes from the include_directive. It's just a fancy wrapper around a hash anyway, and I think it can take its output as input.

So,

JSONAPI::IncludeDirective.new(options[:include]).to_hash ==
  JSONAPI::IncludeDirective.new(
    JSONAPI::IncludeDirective.new(options[:include]).to_hash).to_hash

(I think)

re: 2: that's what unit tests are for :-)

[iMacTia]

Good to know, will give it a try now :)!

[iMacTia]

@NullVoxPopuli unfortunately this doesn't work well with wildcards. All internal options are also lost after calling to_hash 😞

Looks like we'll need some sort of ad-hoc implementation. Does it make sense to have it in the same PR?

[NullVoxPopuli]

2.3.0 :006 > h = JSONAPI::IncludeDirective.new('*').to_hash
 => {:*=>{}} 
2.3.0 :007 > JSONAPI::IncludeDirective.new(h).to_hash
 => {:*=>{}} 

?

[bf4]

@iMacTia @tommiller1 @NullVoxPopuli I wrote some comments in #1843 (comment) that I think relate to this discussion.

but I think the comments beginning with #1333 (comment) deserve a new issue as the original issue is really invalid, and in any case, it's getting hard to summarize the current state of discussion.

I think the summary would be,

Given we're using JSON API
And given a resources and its relationships we're serializing
And given a specific endpoint or place in the code we are serializing the resource
And given we will always want certain associated attributes always to be serialized. (We are not discussing a certain subset of attributes to always serialize).
Then how can be best allow that.
Options:

• In place, as hard-coded options: render json: post, include: [:metrics, :media, :publishers, :author], fields: { media: [:title], publishers: (PublisherSerializer._attributes - :publisher_bio) }
• As defaults in the serializer: has_many :metrics, include: :all, has_many :media, include: [:title], has_many :publishers, exclude: [:publisher_bio], has_one :author, include: :all, which can be overridden by some agreed-upon combination of passing in include and fields.
  • We'd need to make sure this isn't confusing when we have :if, and :unless that are totally different https://github.com/rails-api/active_model_serializers/blob/master/docs/general/serializers.md#associations
• At the adapter level somewhere

[iMacTia]

@bf4 got it, I agree. Let us fix this to_hash thing and then I'll open a new Issue :)
@NullVoxPopuli I made a quick test:

def associations(include_directive = ActiveModelSerializers.default_include_directive)
  return unless object

  # I simply added this line
  include_directive = JSONAPI::IncludeDirective.new(include_directive.to_hash)

  Enumerator.new do |y|
    self.class._reflections.each do |reflection|
      next if reflection.excluded?(self)
      key = reflection.options.fetch(:key, reflection.name)
      next unless include_directive.key?(key)
      y.yield reflection.build_association(self, instance_options)
    end
  end
end

And the result is:

$ rake test
=> 512 runs, 786 assertions, 78 failures, 20 errors, 0 skips

This is because values in JSONAPI::IncludeDirective are nested JSONAPI::IncludeDirective objects, and these objects have an @options property where to store the allow_wildcard option. After calling .to_hash this @options is lost, and tests failing

[NullVoxPopuli]

ah ok. Maybe we need to implement a merge method in the JSONAPI gem. It's owned by one of our maintainers, @beauby :-)

[iMacTia]

nested pool requests 😄 !
@beauby: is it fine for you if we create a PR in your gem to add this method?

[NullVoxPopuli]

@iMacTia @beauby is pretty laid back, I'd just do the PR, and submit it, and see what happens :-)

[iMacTia]

ok, let's for jsonapi as well!
Also, as @bf4 suggested, I'll open a new issue to keep discussing about this :)

[bf4]

Related, test written by Rich 47d7848^...bf4:richmolj-master