Active_model_serializers: Pass :include to Serializer

[richmolj]

Using JSONAPI, I _never_ want to load relationships unless they are also include'd. By default:

class PostSerializer < ActiveModel::Serializer
  has_many :comments
end

Does nothing for me but cause an unnecessary DB hit. Instead, by desired behavior would be:

class PostSerializer < ActiveModel::Serializer
  has_many :comments, if: :comments_included?

  def comments_included?
    self.instance_options[:include].include?(:comments)
  end
end

Unfortunately the include option is passed to the adapter, but unavailable on the serializer.

Can we add include to the options passed to the serializer?

[NullVoxPopuli]

so you don't even want the list of comment ids unless comments_included is true?

[richmolj]

Correct. The list of comment IDs is irrelevant to me, and not always feasible.

Imagine a post that has 100,000 comments I'd rather not load (even just IDs), since I already have a link pointing to /posts/1/relationships/comments, which will handle pagination, etc.

[NullVoxPopuli]

this is a good idea / use case. @bf4, @beauby -- do you have any thoughts on this?

[halorium]

+1
I have the same issue / need for this.

[bf4]

Using JSONAPI, I never want to load relationships unless they are also include'd. By default:

The adapter shouldn't be loading the association unless it's included. That would definitely be a bug.

The adapter asks the serializer for the 'included' associations. The bug would be in how the serializer is answering that question. (So, the issue isn't that the serializer needs an include/d option.)

[bf4]

Appears this is a duplicate of https://github.com/rails-api/active_model_serializers/issues/1552

[NullVoxPopuli]

I'd say so.
closing as duplicate. see: #1552

[NullVoxPopuli]

unclosing, cause the other issue is actually a bug report specific to belongs_to. oops.

[bf4]

Yeah, sounds like same type of bug, probably same place in code

On Wed, Mar 9, 2016 at 6:51 AM L. Preston Sego III [email protected]
wrote:

unclosing, cause the other issue is actually a bug report specific to
belongs_to. oops.

—
Reply to this email directly or view it on GitHub
https://github.com/rails-api/active_model_serializers/issues/1555#issuecomment-194282213
.

[richmolj]

Just some quick clarification:

The adapter shouldn't be loading the association unless it's included. That would definitely be a bug.

This conflicts with your statement in https://github.com/rails-api/active_model_serializers/issues/1325#issuecomment-162016685:

This isn't surprising. If your serializer maps comments, then when the record is serialized, it will serialize comments. The fact that the record hits the DB is a property of the record, not of AMS.

Per the ticket this was just (temporarily) consolidated with, one reason for the conflict may be the difference between belongs_to and has_many. It sounds like belongs_to automatically fetching the relationship is considered a bug, but the same behavior for has_many is not considered a bug (since the IDs can actually be helpful). Is this correct, or do we consider belongs_to automatically fetching the relationship a bug as well?

One way to simplify this is to follow this decision process:

1. If there is an include, relationships and included should be populated (if this causes an N+1 the user would fix this when they fetch the record in the controller).
2. If there is not an include, but there is a link, don't populate relationships. This data is now irrelevant. One real-world example of this is an ember-data async relationship, which will fetch from the link even if the relationships ids are present.
3. If there is neither an include nor a link, then populate relationships (current behavior). I would like to see a way to specifically opt-out of this, but can't think of any good scenarios that really need it atm.

Separately, even if AMS didn't do this automatically under the hood, I'd be happy just exposing the necessary options so I can write a mixin to do it myself. Hence exposing include.

In summary:

1. Is has_many automatically fetching the relationship considered a bug, as is the case with belongs_to?
2. Should AMS automatically follow the include/links/relationships flow noted above?
3. If not, what can AMS do to allow users to manually get this flow (specifying configuration options, etc)?

Does this make sense?

[bf4]

Looks about right. An commuting now. Adapter json should include assoc,
adapter json api should only included association when include param

On Wed, Mar 9, 2016 at 8:46 AM Lee Richmond [email protected]
wrote:

Just some quick clarification:

The adapter shouldn't be loading the association unless it's included.
That would definitely be a bug.

This conflicts with your statement in #1325 (comment)
https://github.com/rails-api/active_model_serializers/issues/1325#issuecomment-162016685
:

This isn't surprising. If your serializer maps comments, then when the
record is serialized, it will serialize comments. The fact that the record
hits the DB is a property of the record, not of AMS.

Per the ticket this was just (temporarily) consolidated with, one reason
for the conflict may be the difference between belongs_to and has_many.
It sounds like belongs_to automatically fetching the relationship is
considered a bug, but the same behavior for has_many is not considered a
bug (since the IDs can actually be helpful). Is this correct, or do we
consider belongs_to automatically fetching the relationship a bug as well?

One way to simplify this is to follow this decision process:

1. If there is an include, relationships and included should be
   populated (if this causes an N+1 the user would fix this when they fetch
   the record in the controller).
2. If there is not an include, but there is a link, don't populate
   relationships. This data is now irrelevant. One real-world example of
   this is an ember-data async relationship, which will fetch from the
   link even if the relationships ids are present.
3. If there is neither an include nor a link, then populate
   relationships (current behavior). I would like to see a way to
   specifically opt-out of this, but can't think of any good scenarios that
   really need it atm.

Separately, even if AMS didn't do this automatically under the hood, I'd
be happy just exposing the necessary options so I can write a mixin to do
it myself. Hence exposing include.

In summary:

1. Is belongs_to automatically fetching the relationship considered a
   bug, as is the case with belongs_to?
2. Should AMS automatically follow the include/links/relationships
   flow noted above?
3. If not, what can AMS do to allow users to manually get this flow
   (specifying configuration options, etc)?

Does this make sense?

—
Reply to this email directly or view it on GitHub
https://github.com/rails-api/active_model_serializers/issues/1555#issuecomment-194325984
.

[NullVoxPopuli]

this sounds like it's also related to #1552 and #1555

[vasilakisfil]

Should that also happen with JSON/Attributes serializers? Because I am working on a similar PR..

Personally I agree to that.

[vasilakisfil]

If I don't specify any include in the serializer, JSON adapter will include '*'. Which means only first level includes.

What should be the default here? Is this still correct (which I don't like personally, I think it should either be '**', meaning include everything recursively unless an include is specified or include nothing unless an include is specified).

[NullVoxPopuli]

** should _not_ be default, cause that would lead to infinite loops, and until people understand how AMS works, I don't think we should expose them to that problem so easily.

[vasilakisfil]

@NullVoxPopuli I disagree. I think it should be the default, and you should have a maximum nest level (which is configurable, but with default value = 3 which is equivelant with '*').

The default '*' is very weird I would say. If you don't like '**', it's better to do it as the op says, completely remove all associations by default.

[NullVoxPopuli]

I guess decisions like this is why we are _strongly_ encouraging people to switch to JSON api

[vasilakisfil]

I don't get it, how JSON api would help on that ?

[NullVoxPopuli]

it has strict defaults, and there is no debating over what should be done, so, with JSON API, if you want relationships, you MUST be explicit about it for each request.

[vasilakisfil]

ok this is JSON API and I think it's a good thing it has strict defaults, it serves a purpose. Regarding JSON adapter, it serves another purpose to give you a flexibility to inherit it and create an adapter for another spec.

So what is the purpose of allowing 1 level associations ? I think it's a poor decision. Why not 2 level and it's 1 level? The correct is either don't allow anything or allow everything (by default).

[NullVoxPopuli]

Or add yet another configurable option, where people could say something like.

AMS.json_adapter_include_default = '*.*.*' # for 3 levels 

[vasilakisfil]

Why don't you like the max nested level configuration ?

I don't know, but I feel kinda weird being in ruby and parsing strings to figure out the configuration.

[NullVoxPopuli]

Just because of potential for infinite loops -- that's all.

My example actually could be passed directly to the include option in the serializer. @beauby and I already worked out the include string parsing :-)

[richmolj]

Correct me if I'm wrong, but I believe the current conversation on nested include configuration is separate from the original purpose of the issue (summed up here). I believe the original issue is JSONAPI-specific (as the proposed solution is conditional based on links) - maybe a separate issue regarding nested configuration?

[beauby]

Joining in a bit late here, but the described behavior is not a bug (serializing a collection of related ids implies querying the corresponding has_many relationship), although we definitely need to provide an easy way to do "link only" relationships, as those have lots of real world use cases.

[richmolj]

Tried to get a PR together and had the following issues:

• @beauby:

serializing a collection of related ids implies querying the corresponding has_many relationship

Agreed. However, the bug would be that AMS tries to serialize a collection of related IDs at all (when not given include). This still might not be a bug, but there should be a way to avoid this behavior, right?

• @NullVoxPopuli @beauby is there agreement on this flow?:

• If there is an include, relationships and included should be populated (if this causes an N+1 the user would fix this when they fetch the record in the controller).
• If there is not an include, but there is a link, don't populate relationships. This data is now irrelevant. One real-world example of this is an ember-data async relationship, which will fetch from the link even if the relationships ids are present.
• If there is neither an include nor a link, then populate relationships (current behavior). I would like to see a way to specifically opt-out of this, but can't think of any good scenarios that really need it atm.
• We actually don't need the raw include passed in, but the section of the include relevant to that specific serializer. Let's say Blog and Post both have comments, and the controller has:

render json: blog, include: {posts: :comments}`

In this example the post comments _should_ be included, the blog comments _should not_. In order to accomplish this we actually need the level of the include tree we're currently processing, not the raw include param.

• I think I would be ready to submit a PR for this, but https://github.com/rails-api/active_model_serializers/issues/1707 would defeat the purpose, and I think there might need to be more substantive refactoring there before I could submit a PR. IOW, what I'm shooting for is (conceptually if not in practice) include_data being a proc that can conditionally choose true/false depending on the include param supplied in the controller.

[beauby]

@richmolj I kinda disagree with

If there is not an include, but there is a link, don't populate relationships. This data is now irrelevant. One real-world example of this is an ember-data async relationship, which will fetch from the link even if the relationships ids are present.

in the general case (example: you have a list of tags that you prefetch on the client side, then whenever you fetch an article, you might want the tags relationship to be populated, though you wouldn't need to include them).
I think AMS aims to be sufficiently flexible to accommodate both workflows.

[richmolj]

@beauby good use case! I agree this should be supported, makes good sense.

Personal preference is populating relationships is opt-in instead of a default (like include works now), but as long as I have the option one way or the other 👍