Add include_data :if_sideloaded

[richmolj]

Purpose

Currently, AMS will populate relationships even when said relationships are not part of the include directive. This is problematic, as it causes activerecord queries to fire to populate an unnecessary part of the payload.

Changes

This now adds only one new variable name, include_slice. This is the slice of the include directive hash being processed by the current serializer. All the changes to lib/active_model/serializer/associations.rb and lib/active_model_serializers/adapter/json_api.rb are just passing that slice around so we can access it when we finally decide whether or not to have the data key in the response. The changes to lib/active_model/serializer/reflection.rb access that variable to make the decision.

Users now opt-in to this pattern using the current DSL. The current DSL conditionally includes the data key in the response via include_data true/false:

class PostSerializer < ActiveModel::Serializer
  has_many :comments do
    include_data false
  end
end

We now add another option :if_sideloaded. This means we only include the data key when the relationship is being sideloaded:

class PostSerializer < ActiveModel::Serializer
  has_many :comments do
    include_data :if_sideloaded
  end
end

Instead of specifying this for every relationship in every serializer, users can make this the default:

ActiveModelSerializers.config.include_data_default = :if_sideloaded

Caveats

• I believe there is broad consensus that include_data_default should be :if_sideloaded by default. However, this would break backwards compatibility, so I have avoiding doing so. If I can get consensus we are OK with the breaking change, I will make the update.
• There is an existing issue with AMS, that in order to figure out links we need to execute the block given to the relationship. This block may provide a custom way to fetch the data. This means if you have an association with an in-line block customizing that association's value, this PR doesn't really help you - the DB query is firing in any case. Fixing this issue is out of scope for this PR.

Related GitHub issues

• Add include_data :via_include_parameter #1797
• Support JSONAPI include params with links/loading #1720
• Pass :include to Serializer #1555

Additional helpful information

• :if_sideloaded is the default behavior of jsonapi-resources. In fact, it's the only supported behavior.
• Since this has been a source of confusion in the past, I'd like to point out AMS already supports include_data true/false. We are adding one more option, :if_sideloaded.
• This adds a variable include_slice to the jsonapi adapter. Why do we need the current slice instead of the whole directive? Because multiple entities can 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. include_slice is the slice of the overall include directive relevant to the current serializer.
• include_slice is somewhat of an eyesore, but cannot go away without fundamentally refactoring the way adapters and serializers work.

[mention-bot]

@richmolj, thanks for your PR! By analyzing the annotation information on this pull request, we identified @beauby, @bf4 and @groyoh to be potential reviewers

[richmolj]

@bf4 @NullVoxPopuli what's the status here? I believe I can sum this up as:

• This code introduces additional complexity with include_slice.
• The only way to avoid that additional complexity is to fundamentally rework some of the innards of AMS. Some relevant work is going on here and I've noted some inherent problems with the jsonapi adapter here.

So, as I understand it, we can either merge now or wait for a fundamental refactoring of the library. I'd prefer to merge now, as from my perspective using this library for jsonapi is fundamentally broken without this fix, and any refactoring should be paying attention to these tests. There's not a simple workaround or monkey patch, so I'm having to continually maintain my fork.

If there are any outstanding issues I've missed that don't require a fundamental refactor, please let me know so I can tackle them.

[NullVoxPopuli]

yeah, I think we need to wait until the architecture is a bit better. We don't want to add more mess for us to clean up

[NullVoxPopuli]

@beauby had a neat api for doing this: beauby/jsonapi#38

[NullVoxPopuli]

I'm approving this, knowing that the API will change a bit for relationships in the future.

[NullVoxPopuli]

It's somewhat gross, but people need the functionality. We'll just be sure to explain how to change to the new API when we get that done.

[masciugo]

My Movement model belongs_to :payer. In order to not fall into N+1 query problem I had to define my serializer the following way:

class MovementSerializer < ActiveModel::Serializer
  belongs_to :payer do
    {
      type: 'players',
      id: object.payer_id
    }
  end

so that my Ember JSONAPI serializar is happy with JSON:

....
"relationships": {
      "payer": {
        "data": {
          "type": "players",
          "id": 6
        }
      }
}

How can I obtain the same using include_data option?

[axsuul]

Would also like to know as well. The documentation has been quite confusing for me to be honest. Also currently how I'm doing it is different serializers depending on what sort of associations I want and not sure how to use include:

class UserSerializer < ActiveModel::Serializer
  attributes :name, :email
end

class CurrentUserSerializer < UserSerializer
  has_many :organizations do
    OrganizationSerializer.new(object).as_json
  end
end

Not sure if this is the proper way

[bf4]

@masciugo @axsuul It sounds like you both have new and different issues. There're all sorts of details that I'd need to help you so that I can understand what you're trying to do.

If you're trying to avoid the N+1 on a belongs_to with JSON API, that's not fixed, yet.

It's unclear from your comments if you're doing what this PR documents

  has_many :comments do
    include_data :if_sideloaded
  end

[richmolj]

I believe they'd like the relationship resource identifier to appear, even though the data is not sideloaded. I'm assuming that's in the purview of the N+1 belongs_to work.

…

On Wed, Mar 8, 2017 at 3:43 PM Benjamin Fleischer ***@***.***> wrote: @masciugo <https://github.com/masciugo> @axsuul <https://github.com/axsuul> It sounds like you both have new and different issues. There're all sorts of details that I'd need to help you so that I can understand what you're trying to do. If you're trying to avoid the N+1 on a belongs_to with JSON API, that's not fixed, yet. It's unclear from your comments if you're doing what this PR documents has_many :comments do include_data :if_sideloaded end — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1931 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADX4IMtZx7U4dSUrrDFZTMQh0loGRSHks5rjxLzgaJpZM4J_Pvd> .

[axsuul]

@bf4 My issue has been resolved, sorry, thanks