Support JSONAPI include params with links/loading

[richmolj]

Purpose

The current implementation does support conditionally sideloading relationships based on the 'include' URL param. However, omitting the relationship still loads the relationship (to populate the type/id
'relationships' payload), somewhat defeating the purpose.

Changes

This changes the flow to:

1. If the relationship is included, load it and include it the response.
2. If the relationship is not included but there is a JSONAPI link,
   include the link in the response but do not load the relationship or
   include data.
3. If the relationship is not in the URL param and there is no link, do
   not include this node in the 'relationships' response.

Caveats

This API is opt-in to support users who always want to load relationships data. To opt-in:

class BlogSerializer < ActiveModel::Serializer
  associations_via_include_param(true) # opt-in to this pattern

  has_many :tags
  has_many :posts do
    link :self, '//example.com/blogs/relationships/posts'
  end
end

Related GitHub issues

Fixes #1707
Fixes #1555
Related: #1325

Additional helpful information

The current_include_tree edits in json_api.rb are to pass the current nested includes. This is to support when multiple entities have the same relationship, e.g. /blogs/?include=posts.tags,tags should include both blog tags and post tags, but /blogs/?include=posts.tags should only include post tags.

JSONAPI include parameters (http://jsonapi.org/format/#fetching-includes).

[richmolj]

Also related: #1552

[richmolj]

Just made an edit after realizing this wasn't supporting association blocks. To fix this, I had to change the API from:

has_many :comments do
  Comment.where(hidden: false)
end

to

has_many :comments do
  load_data { Comment.where(hidden: false) }
end

This is because we have to execute the block to figure out links and meta, but the data should not actually be loaded when we fire that block. load_data now lazily executes the actual data fetching and IMO better matches the rest of the block API (include_data, links, etc).

[richmolj]

@NullVoxPopuli @bf4 @beauby seems like we were starting to agree on this direction in #1555, does this satisfy your concerns? If not, I'm happy to make the desired changes.

Without this patch I cannot use this wonderful gem due to uncontrollable N+1's, so please let me know what I can do to help 😄

[NullVoxPopuli]

@richmolj can you add a test (or point me to one) that shows having include data without links?

[richmolj]

@NullVoxPopuli the test for block relationships is an example where the relationship is include'd and there are no links present. Is what you are looking for? If so, would you like me to add a separate test explicitly just for this scenario (unrelated to blocks)?

[NullVoxPopuli]

maybe show mixed usage?

like, what if I always want relationshipA, but relationshipB and
relationshipC to be included via includeParams?

On Mon, May 23, 2016 at 12:20 PM Lee Richmond notifications@github.com
wrote:

@NullVoxPopuli https://github.com/NullVoxPopuli the test for block
relationships
https://github.com/richmolj/active_model_serializers/blob/da157c3536d1ed276e5e94cb2d17cfbe70b0921c/test/adapter/json_api/include_param_test.rb#L130-L138
is an example where the relationship is include'd and there are no links
present. Is what you are looking for? If so, would you like me to add a
separate test explicitly just for this scenario (unrelated to blocks)?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#1720 (comment)

[richmolj]

@NullVoxPopuli I currently do this in the controller (FWIW I think this same functionality would need to be present independent of this PR if you want a default include):

class PeopleController < ApplicationController
  def index
    render json: Person.all, include: includes
  end

  def default_includes
    { relationship_a: {} }
  end

  private

  def includes
    includes = ActiveModel::Serializer::IncludeTree::Parsing
      .include_args_to_hash(params[:include])
    includes.merge!(default_includes)
    includes
  end
end

Now, you might instead be saying, "what if we want to have the relationships payload for relationship_a always populated with IDs without being include'd?" And this PR actually does not support that. If you opt-in (associations_via_include_param(true)), you will only ever get relationships populated if there is a corresponding include (IMO this is the 95% scenario).

My thought was conditionally toggling relationships would require a separate discussion/PR since you might want to do this independent of any include param functionality.

[NullVoxPopuli]

ah ok, cool. thanks for explaining that. In that case, I'm good with this PR.

I'm glad this is now documented, so if anyone has questions about this in the future, we can point at this PR.

[NullVoxPopuli]

can you add a change log entry?

[richmolj]

@NullVoxPopuli done! I also updated the docs for block relationships. How's that look?

Not sure what is up with AppVeyor failing? I just rebased off of master and have no conflicts.

[NullVoxPopuli]

is this really a breaking change? or can load_data be omitted?

[richmolj]

Unfortunately load_data is required when you opt-in to this feature (see #1720 (comment) for explanation). I could make it so when you opt-out it's still the old API, but this seems confusing to me.

My hunch is you'll probably want this API (running the relationship block but lazy-loading the data) for other features as well, though I can't think of any top of my head.

[bf4]

@richmolj I don't want to ask you to do a lot more work at this point, but would you mind sketching out some docs with usage, perhaps modify existing docs? (I'm just going through issues and am not reading all the code, and I didn't see a clear example.. forgive me if it's clear somewhere and I missed it)

[blmundie]

+1. I need to be able to only do links for some relationships unless it's included in the include list. Right now if you set include_data to false you can't include it.

[remear]

@richmolj This needs a rebase. Note that, depending on timing, this might need more than rebase since we have several PRs queued to be merged that could cause this one to conflict again before we can merge it. It would be helpful if you could keep an eye on this PR and make sure it stays up-to-date with the latest master.

[richmolj]

@remear rebased. I will try to keep an eye on it, please let me know what else I can do.

[richmolj]

@groyoh thanks for your input! I appreciate that you've been looking along these lines for a while.

That said, I think there is some confusion. This PR does exactly what the responders to both of those links recommend - something that's not possible with the current version of this gem! Assuming you have links in your serializer, we will render relationships members with links but without data. The only time relationships will be completely empty is if you aren't adding links, which is really unrelated to this PR. You can see that relationships are present when links are present in this test.

Side note: this will actually render something like {relationships: {pets: {}}} when no links are data are present. My hope here is this expresses that the resources has pets relationship, we just aren't saying anything about it (no data or links, which means we aren't saying the relationship is blank, just that data is omitted). Ember-Data honors this FWIW.

Also having to use load_data breaks the main purpose of inline associations.

I'm totally receptive to not breaking backwards compatibility. If there is any other alternative here you suggest (to be able to grab the links content without running the rest) I am happy to make that change. However, I don't see how this "breaks the main purpose of inline associations". It's a minor change from

has_many :comments do
  Comment.active
end

to

has_many :comments do
  load_data { Comment.active }
  # or an alternative api:
  -> { Comment.active }
end

The functionality remains the same.

[richmolj]

To clarify the load_data issue a bit more: the premise is we need include_data to be conditional based on the ?include-params. So an API like this, where include_data is a conditional proc:

has_many :comments do
  include_data { |include_directives| include_directives.key?(:comments) }
  Comment.active # sql query fires
end

Would fire the Comment.active code even if include_data evaluates to false (the block gets instance_exec'd here). So I wrap the Comment.active code in a proc as well to delay the execution. Open to alternatives as long as this functionality can be achieved.

[NullVoxPopuli]

I think what this means is that we need to redesign relationships as a
whole. The include data thing feels like a bandaid, imo

On Tue, May 31, 2016, 10:43 AM Lee Richmond notifications@github.com
wrote:

To clarify the load_data issue a bit more: the premise is we need
include_data to be conditional based on the ?include-params. So an API
like this, where include_data is a conditional proc:

has_many :comments do
include_data { |include_directives| include_directives.key?(:comments) }
Comment.active # sql query firesend

Would fire the Comment.active code even if include_data evaluates to false
(the block gets instance_exec'd here
https://github.com/rails-api/active_model_serializers/blob/master/lib/active_model/serializer/reflection.rb#L76-L81).
So I wrap the Comment.active code in a proc as well to delay the
execution. Open to alternatives as long as this functionality can be
achieved.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1720 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAMJarLGrUqyVZbawBeXOsz1dws7qVi_ks5qHEkQgaJpZM4IaKqH
.

[richmolj]

@NullVoxPopuli @groyoh could you please elaborate on what you are looking for here? I am happy to help in whatever way can, but I haven't heard any alternatives to breaking backwards compatibility. IMO this change is a good thing independent of this PR, since delayed execution will provide a more flexible API going forward.

The best I can think of is to make the old-style work, it will just be an unnecessary performance hit. I could do this, then attach a deprecation warning whenever the block returns something other than :nil?

Just to give my personal perspective, this is minor change that turns the gem from unusable (due to inefficient queries or N=1's) to working perfectly for my use case (?include-style API).

In the meantime, I've rebased again.

[groyoh]

Just to give my personal perspective, this is minor change that turns the gem from unusable (due to inefficient queries or N=1's) to working perfectly for my use case (?include-style API).

@richmolj isn't the issue actually the fact that you're firing SQL queries within your serializer? And I wouldn't call a breaking change "minor". 😉 For example, that would imply that we change most of our serializers (and we do have a lot) in dozen services that we have in my company.

The best I can think of is to make the old-style work, it will just be an unnecessary performance hit. I could do this, then attach a deprecation warning whenever the block returns something other than :nil?

Why would it be a performance hit? Also I'm not sure if a deprecation warning may be needed here. I mean, I see the use case where people would want to have inline association:

has_one :user { object.author }

more prominent than using:

has_one :user do
  load_data { object.author }
end

[NullVoxPopuli]

nothing against you, @richmolj, but I feel like this PR doesn't address the root of the problem.

What it comes down to, is we really need to redo our relation classes / workflow -- allow lazy evaluation, integrate with ActiveRecord (esp for belongs_to).

I want the N+1 problem solved as much as you do, but we need to do it the right way.

[NullVoxPopuli]

We kinda have a discussion going here: #1750 (comment)

[richmolj]

@NullVoxPopuli yes we are actually in total agreement 😄. I'm not trying to ram this PR through, I'm trying to ask what your desired "right way" is. I still haven't seen an alternative to load_data expressed anywhere; I've tried to reiterate the problem in the linked issue: #1750 (comment). Alternatively, it's cool to let me know I'm wasting my time here and should stop rebasing.

@groyoh based on your comments I think there are maybe some misunderstandings about this PR I can hopefully clear up:

isn't the issue actually the fact that you're firing SQL queries within your serializer?

Nope, it's that AMS is forcing me to add stuff to my response that I don't want there (which happens to cause a DB hit). Take jsonapi-resources: if I do not include a relationship, then data is not populated with id/types. This is the behavior I would like, but AMS does not allow a conditional include_data based on the ?include parameters.

Why would it be a performance hit?

See #1720 (comment) and #1750 (comment). Even if I only want to load links, I am still forced to run the inline-association getter code, which would fire SQL and cause the performance hit.

I mean, I see the use case where people would want to have inline association...more prominent than using...

Sorry I don't really understand this comment. Both the code blocks support the same inline association behavior. One just allows us to get information about the association (include_data, links, etc) without loading the association itself. If you're saying users would prefer to to type less characters I agree, I just don't see how to avoid the performance hit in that case.

[NullVoxPopuli]

@richmolj I see what you're saying about the block just always evaluating, cause our current api doesn't provide a way to not load that data.

hmm. I'm back on the fence now.

@bf4, @beauby any thoughts?

[richmolj]

@NullVoxPopuli while waiting: is the only issue load_data? My thought is I can remove load_data and make this backwards-compatible...everything will work, there will just be an unnecessary performance hit when using inline associations.

That performance hit seems to be a separate issue from this PR, as #1750 has the same problem. Maybe we could merge in this functionality and tackle that issue separately?

[NullVoxPopuli]

@richmolj I'd be in favor of that. Good thinking. Could you run some benchmarks on how big of a performance impact it would be?

[richmolj]

@NullVoxPopuli I'm not sure I can, since it's really dependent on what code the inline-association block is running (Comment.active, etc). FWIW the scenario you get a performance hit is:

• You opt-in to this ?include-style API
• An inline-association is not in the include-parameter list, which means we want to add links if present.

[beauby]

So I've been thinking about this use-case for a while (sorry I haven't chipped in earlier).
The behavior I'd like would be an adapter option to enable this feature (and possibly a global config to enable it by default).
Now the main issue here is that in the current DSL, computing the link implies computing the actual value of the association if it is overridden (which is a slightly different problem than the optim on belongs_to assocs).
Two options (at least) to fix that:

1. as suggested by our very own @richmolj, change the DSL slightly so that overriding an association is done via a method call from within the block definition of the association (which allows to compute it lazily),
2. move away from the block definition style altogether (i.e. keep the block for overriding only, and move all the assoc configs to options on the has_many/belongs_to call – those options would take lambdas as values).

Is there a simpler solution I'm not seeing here?

[richmolj]

@beauby think you got it, thanks! Either of those options sound good to me. Not necessarily proposing this, but removing inline associations and relying on method definitions would also get the job done.

The behavior I'd like would be an adapter option to enable this feature (and possibly a global config to enable it by default).

This is currently enabled in your serializer:

class PeopleSerializer < ActiveModel::Serializer
  associations_via_include_param(true) 
end

I put this at the serializer level instead of adapter because I can see the use case where you don't want this everywhere (particularly when you have many already-written serializers, but want to use this pattern for new ones). In my app I have an ApplicationSerializer that enables this, and it is in-effect globally enabled as everything subclasses that. Hope this fits your thinking.

@NullVoxPopuli I have rebased and refactored the code to support the backwards-compatible inline-association style. There's still the performance hit if you don't use load_data, but it works and there's a test added specifically for this case. So could tackle that issue separately if you'd like.

@bf4 I've added some docs as well, hope this helps.

[richmolj]

@NullVoxPopuli just a heads up: rebased again, gtg.

[bf4]

This is already an AMS feature. I'd like to review this more carefully.

I'm using 0.10.0 with include and fields without issues

[richmolj]

@bf4 from the PR description:

The current implementation does support conditionally sideloading relationships based on the 'include' URL param. However, omitting the relationship still loads the relationship (to populate the type/id
'relationships' payload), somewhat defeating the purpose.

Links was the other issue.

Is your issue with documenting how this is explained, or the content itself?

[bf4]

This PR has always made me a little nervous so I've mostly avoided reading it and relying on others.. but now I'm a little more concerned about some of the design choices, so will take a look. It's hard to balance trying to do what's right for the code base and wanting to use the solutions and work-product in pull requests. So, my interest right now is to make this PR is good for AMS, not just a good feature or good code.. that make sense?

[bf4]

I kind of have a hard time following what the changes are trying to do in the PR mostly because of the naming, I think

• What does _associations_via_include_param connote?
• What's the difference between include_directive and current_include_directive?
• What's the difference between current_include_directive and full_include_directive
• Why are there two include directives now?
• Why is there a param include_directive introduced that defaults to {} when no arg passed?
• What does load_data connote?
• What's the difference between include_data and load_data?
• Why is the consequence of include_data? a choice between @_load_data.call and include_data_for?

[richmolj]

Good questions! I absolutely agree the naming gets confusing here and glad to take any suggestions.

What does _associations_via_include_param connote?

That you're opting in to this feature. The biggest reason why you might not want to opt-in is if you want the relationships payload to always include id/type regardless of the ?include parameter. The biggest reason you want to opt-in is to avoid the performance penalty fetching the id/types will incur. IMO this is the 80%+ use case and is the current behavior of json-api-resources

What's the difference between include_directive and current_include_directive?

include_directive is the current code - this is the IncludeDirective passed to render. It looks something like { posts: { comments: { author: {} }, tags: {} }, tags: {} }. current_include_directive is the slice of that hash being processed by the current serializer (so CommentSerializer would see { author: {} }. We need to differentiate between these two because the same serializer might be used multiple times at a different nesting level (like tags in this example).

What's the difference between current_include_directive and full_include_directive

Good question. full_include_directive is the same as what I've noted above as include_directive - the current behavior of the gem, the full directive passed to render. I could make it full_include_directive everywhere if that would make it more readable.

Why are there two include directives now?

Explained above.

Why is there a param include_directive introduced that defaults to {} when no arg passed?

This is the current code actually...it's just providing a default for when no include arg is passed to render. And this default can be customized, I think so people can default to including all relationships (*).

What does load_data connote?

We're in the relationships block here, where this gem provides functionality like link - "here's the proc to fire when fetching links data". Currently the return value of the block can be used to customize the value of the relationship:

class PostSerializer < ActiveModel::Serializer
  has_many :comments do
    link :related do
      href "/posts/#{object.id}/relationships/comments"
    end

    Comment.not_deleted
  end
end

The problem is, this PR wants to process that link proc without firing that DB query that's going to get all undeleted comments. So we wrap that data fetching part in a proc, this way we can run the block but delay execution of data fetching. Open to any alternate names here, I've also suggested just forcing the user to return a proc.

More discussion on why we need to delay the execution is in the comments above.

What's the difference between include_data and load_data?

include_data is current code - it means, "should we include the data part of the payload for this relationship?". Currently users set this to false/true. load_data is explained above - the proc to fetch the relationship.

Why is the consequence of include_data? a choice between @_load_data.call and include_data_for?

If the relationship data key should be in the response (include_data?), we need to load this relationship. So either use a custom proc (inline-relationship functionality of AMS), or just go through the normal codepath.

Hope this helps, @bf4, appreciate your time looking at this!

[bf4]

Ok, thanks for understanding. I'm going to block this implementation while we consider either a better one or an intermediate that let's you patch it yourself while we work on it in the gem

B mobile phone

On Jun 10, 2016, at 8:41 AM, Lee Richmond notifications@github.com wrote:

In docs/general/adapters.md:

@@ -194,6 +194,43 @@ The user could pass in include=**.

We recommend filtering any user-supplied includes appropriately.

+#### Include Parameters
+
+You may wish to include related resources based on the include request
+parameter (See fetching_includes. You can opt-in to this pattern:
+
+```ruby
+class PostSerializer < ActiveModel::Serializer

• associations_via_include_param(true)
  So, the problem this solves is wanting to make a relationship link without loading the relationship, or to render the relationship id and type without loading the relationship?

Quick follow up note I hope clears up confusion - making the link without loading the relationship is kinda an independent issue (this is why @NullVoxPopuli and I arrived at making this code backwards-compatible, and just taking that performance hit in this scenario - so it can be addressed independently of this PR). This same problem exists for #1750 (I tried to sum up in #1750 (comment)). The way inline-relationships are loaded is an issue that impacts this PR, but it's not specific to or caused by this PR, if that makes sense.

I don't personally use inline associations so it's not actually applicable to me, was just trying to solve this use case at the same time when I ran into it. As you can see I am obsessed with unnecessary DB hits 😆

Avoiding the automatic rendering of /id/type conditionally based on the ?include parameter - and the performance hit that comes with that automatic rendering - is the goal of this PR.

Once again, I really do appreciate your time looking at this @bf4. As this gem is unusable to me without this, please let me know how I can help! Even if that is stepping back and letting you tackle this differently 😄

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

[richmolj]

@bf4 cool. Is there a place I can follow that progress, so I can figure out when I'll be able to use AMS?

[bf4]

I was thinking of the discussion you're already aware of #1750 (comment) but admit it's not active...

[bf4]

I think we can re-use your tests to drive the feature

[NullVoxPopuli]

Re: that PR/issue, I think we need to discuss what to do about it, or (if everyone's OK with it, one of us could just fix 'the problem') , but I know there was a desire to re-architect relationship building. And I think there may have been some blocker to that work? I don't remember

[vasilakisfil]

Hi, I just hopped in to confirm that:

The following doesn't work with ember-data (ember-data does not retrieve the relationships).

{
    "data": {
        "id": "1",
        "type": "users",
        "attributes": {
            "name": "Example User",
            "email": "example@railstutorial.org",
            "created-at": "2016-07-15T14:51:57Z"
        },
        "relationships": {
            "microposts": {
                "data": [],
                "links": {
                    "related": "/api/v1/microposts?user_id=1"
                }
            }
        }
    }
}

The following does:

{
    "data": {
        "id": "1",
        "type": "users",
        "attributes": {
            "name": "Example User",
            "email": "example@railstutorial.org",
            "created-at": "2016-07-15T14:51:57Z"
        },
        "relationships": {
            "microposts": {
                "links": {
                    "related": "/api/v1/microposts?user_id=1"
                }
            }
        }
    }
}

The difference is that the former returns a data attribute inside the relationship with an empty array as a value. I guess ember-data figures out that there is no data so it doesn't send any request even if you explicitly say so.

In my opinion although this is not entirely wrong (having an empty array in data attribute and at the same time provide links), I really do think that ember-data's behavior is more correct. Ideally, if you only want to provide links (imagine having millions of associated data, links is the only way to go) it should be possible to skip data attribute entirely.

At the same time a lot of people use this gem along with ember-data and this can lead to frustration. Myself, not a newbie in Rails (and Ember either!) lost 2 hours to figure out what's going on. I really think we need to address this.

[bf4]

Great work explaining the problem. Shouldn't be too hard a fix I'm personally spread really thinly right now, so anyone could pick up

[richmolj]

@bf4 someone has already picked it up, it is literally the PR you are commenting on 😄. Or #1797, which is a somewhat worse solution but simpler code. If anyone could give feedback about changes they would like to the PR, I would be happy to make them, but so far I really haven't heard anyone propose an alternate solution that would work. So I'm not sure what there is to do other than wait around.

I've been continually rebasing #1797 as my app must run off of this fork.

(@vasilakisfil I think you may have meant to respond to this comment in a separate but related PR)