Closed bdmac closed 8 years ago
What version of AMS are you on? Can you help us by filling out some of the info in https://github.com/rails-api/active_model_serializers/blob/master/.github/ISSUE_TEMPLATE.md?
I can't find the issue, but I agree with you that JSON API should not include any relationships by default.
module Associations
extend ActiveSupport::Concern
DEFAULT_INCLUDE_TREE = ActiveModel::Serializer::IncludeTree.from_string('*')
def associations(include_tree = DEFAULT_INCLUDE_TREE)
module ActiveModelSerializers
module Adapter
class JsonApi < Base
def initialize(serializer, options = {})
super
@include_tree = ActiveModel::Serializer::IncludeTree.from_include_args(options[:include])
# snip
serializers.each { |serializer| process_relationships(serializer, @include_tree) }
# snip
def process_relationships(serializer, include_tree)
serializer.associations(include_tree).each do |association|
process_relationship(association.serializer, include_tree[association.key])
end
end
such that
>> ActiveModel::Serializer::IncludeTree.from_include_args(nil)
=> #<ActiveModel::Serializer::IncludeTree:0x007fb78b438718 @hash={}>
>> ActiveModel::Serializer::IncludeTree.from_string('*')
=> #<ActiveModel::Serializer::IncludeTree:0x007fb78b4a8130 @hash={:*=>{}}>
>> PostSerializer.new(Post.new(comments: [Comment.new(id: 1)])).associations(ActiveModel::Serializer::IncludeTree.from_include_args(nil)).to_a
=> []
>> PostSerializer.new(Post.new(comments: [Comment.new(id: 1)])).associations( ActiveModel::Serializer::IncludeTree.from_string('*')).to_a
=> [#<struct ActiveModel::Serializer::Association name=:comments, serializer=#<ActiveModel::Serializer::CollectionSerializer:0x007fb8c89289d8 @root=nil, @object=[#<Comment:0x007fb8c892a8f0 @attributes={:id=>1}>], @serializers=[#<CommentSerializer:0x007fb8c8928078 @object=#<Comment:0x007fb8c892a8f0 @attributes={:id=>1}>, @instance_options={:serializer_context_class=>PostSerializer}, @root=nil, @scope=nil>]>, options={:include_data=>true}, links={}, meta=nil>, #<struct ActiveModel::Serializer::Association name=:blog, serializer=#<BlogSerializer:0x007fb8c8922da8 @object=#<Blog:0x007fb8c8923f00 @attributes={:id=>999, :name=>"Custom blog"}>, @instance_options={:serializer_context_class=>PostSerializer}, @root=nil, @scope=nil>, options={:include_data=>true}, links={}, meta=nil>, #<struct ActiveModel::Serializer::Association name=:author, serializer=nil, options={:include_data=>true}, links={}, meta=nil>]
so it looks like it should be working..
Doesn't sound like there's a bug here to address and without more information we can't look into this further. As for optimizing relationship inclusion, I believe a few other pull requests are working to improve areas around this concern, including #1426.
Sorry for not replying earlier. This was more of an optimization question than a bug. If you need to generate a list of a models relationships you can do it a few ways. AMS is using include which loads all the data from the associated table for possible serializarion. JSONAPI would only need that data if you told it to include the related model explicitly. It would only need the IDs otherwise for the default case. Also for a belongs_to the ID needed for the reference is actually already naturally included in the query for the parent model so no additional query is needed at all in that case. Not sure if the include tree customization really applies or helps here since in my case I'm not including the relationship directly anyways, AMS is automatically pulling in the relationships though to fulfill the JSONAPI contract.
@bdmac
AMS is using include
I don't believe it is using the ActiveRecord::Base.include query method. Would you mind pointing me to it in the code?
I'm sorry that's probably true that it's not explicitly using it I just meant implicitly it has a similar effect. I'm assuming it's actually just using the AR association accessors. But if you are just pulling those in for JSONAPI relationships the only data you need are type and id. Type is available without a query so the only thing we need to query for is id.
Using the association accessors will trigger N+1 queries for each loaded association unless you explicitly AR include them. Note that the purpose of including in that case differs from including in JSONAPI which would imply you want the entire entity included in the payload's includes key. Even AR includes won't save you if you access a belongs_to association as I don't believe AR includes saves you any queries there.
The alternatives if you are only needing type/ID for JSONAPI relationships would be to use the *_id(s)
AR methods. These should have a performance benefit. For belongs_to using association_id
does not perform any queries and just pulls it from the owning model's loaded data. For has_many you can use association_ids
. This will generate a query but will still be a lot faster because AR won't turn the results into a bunch of models that you don't actually need to use since all you need is the ID array. The type can be fetched from the association reflection metadata.
Great summary of the situation @bdmac.
I've noticed that when I define associations in my serializers AMS winds up loading up a ton of data to meet the JSON API spec. Many of these relationships don't actually need to be loaded, specifically any belongs_to relationships.
I get that we need to run a query to grab the resources in a has_many (or even a has_one) but if you have a belongs_to relationship the ID of the associated resource lives on the parent resource and unless it is also included, loading the associated resource is inefficient. Even in the case of has_many relationships it would seem like we could just load the IDs with pluck(:id) instead of loading up the entire model object unless include was specified.
Are there any plans to optimize any of this? I currently find myself playing whack-a-mole with AMS queries. This is made especially hard if your API supports params[:include] since you don't really know what a client may request.