Performance improvement with associations using includes?

[jfelchner]

Just wanted to throw this in here for someone with more knowledge about AMS than I.

It seems as though it would be a prudent thing to do to check to see if the thing being serialized in an ActiveRecord::Relation and, if so, fire off an includes for the set of associations on the serializer.

This doesn't seem to me to be something that should be handled at a higher level since we're specifying the associations we want in the serializer. Doing it a level up just means tons of duplication.

I've noticed AMS being the cause of hundreds of queries being fired when it should just be making dozens. It seems to be causing a ton of timeouts for us.

So if we had:

class PostSerializer < ActiveModel::Serializer
  has_many :comments
  has_one   :author
end

Then AMS would attach includes(:comments, :author) before it fires the method that causes the query to be executed on the DB.

Thoughts?

[beauby]

I really think AMS should not be responsible for any of the data-related stuff. However, I think the kind of queries that would implicitly be fired by AMS through ActiveRecord should be documented, as well as suggestions and/or helper methods to help preload stuff with AR.

[bf4]

@beauby +1

[jfelchner]

@beauby I agree up to a point. I'm looking at this from the perspective of what is the most common use case as well as what does the user interface look like.

As it currently stands, the include option can (and should) be specified from the controller. However, this means that the data manipulation for including associations can either be:

• done on the object there in the controller before it's turned into an AMS object
• specified in the serializer as an option.

Doing it in the controller is messy and a ton of boilerplate that should be avoided (eg having to specify associations twice). If instead we had an option (add_included_associations or whatever) which was opt-in, then newbies who don't know any better would never turn it on. In fact, the fact that so many people use AMS and no one has brought this up until now is a testament to how few people actually think about this. Because odds are they're not doing the includes before they get to the serializer.

If it's specified in the serializer, it limits whether it can be customized per controller action.

The serializer has all the information it needs to determine whether an include is the right thing to do (including the serializer's include and the association names. Forcing the user to know or care about that is unnecessary.

Personally I think that at some point there's going to need to be a Serializer::ActiveRecord which includes the base serializer and does special things to the object before its serialized. That way you can keep data manipulation (and other AR-specific stuff) separated out from the actual PORO serialization. But that's a digression. All I care about for this issue is AMS transparently doing a basic optimization to the queries.

[bf4]

Perhaps provide a hook of some sort where you can 'prepare' an object for an association?

[jfelchner]

@bf4 yeah that would work, but still has the problem of being opt-in. Whereas if you're serializing an AR object with associations included (in the serialized hash), I can't think of a single time where those associations should not be included.

[bf4]

Polymorphic handlng of poros vs arecord?

[jfelchner]

@bf4 I think that would be the right approach yeah. :)

[bf4]

I'd just hope we remain conservative in what we promise and easy to customize, so as to keep maintenance needs low, mostly at the interface level, and minimize bugs...

Expanding the scope to manage AR associations worries me.

[bf4]

Ooh! The poro is activemodel::serializer and the ar is activerecord::serializer!!!

[jfelchner]

@bf4 that's what I was outlining above yeah. :) if it seems like that's the best option I think we go for it. ❤️

[beauby]

I think there should be a helper or a separate gem in order to fetch "just the right data" depending on a serializer and serialization options. Thoughts @joaomdmoura @bf4 @NullVoxPopuli?

[bf4]

would a reasonable api be just to document the following?:

If you would like add joins and includes or otherwise extend the resource to be serialized,
simply alias_method :unscoped_object, :object and define object as e.g. def object; unscoped_object.includes(:user).joins(:posts); end etc.

I think we could just def object; super.includes(:user).joins(:posts); end but I'm not 100%

I was going to add a method to the serializer, but realized this might be the cleanest way, though not the most intuitive, and it's still declarative and explicit.

[scttnlsn]

I have some endpoints where the client can pass in various AMS include options (via query string) to embed any needed relationships. In order to prevent over-fetching data from the database I need to parse the include string and then call the includes method on the ActiveRecord::Relation accordingly.

I think a separate helper/Gem that parses the AMS string (i.e. 'comments,author.profile' and translates it into something you can pass to includes (i.e. includes(:comments, author: :profile)) is fine here.

Does AMS internally expand the string form to a simple nested hash? Perhaps exposing that publicly could help here.

[NullVoxPopuli]

does ActiveModel::Serializer::IncludeTree::Parsing.include_string_to_hash(..) do what you want?
https://github.com/rails-api/active_model_serializers/blob/master/lib/active_model/serializer/include_tree.rb#L19

[scttnlsn]

@NullVoxPopuli That looks exactly like what I need. Thanks!

[NullVoxPopuli]

woo!

[beauby]

Closing this as AMS will not do auto-including at ActiveRecord level. Moreover, one can do

includes = ActiveModel::Serializer::IncludeTree::Parsing.include_string_to_hash(include_string)
# Or 
# includes = JSON::API::IncludeDirective.new(include_string).to_hash
# if using the `jsonapi_parser` gem.
render json: Post.includes(includes)

[NullVoxPopuli]

oh hey, that's a good idea.

[ram535ii]

@beauby That solution looks interesting, what would the format of include_string have to be? (I'm trying to pass it from the frontend.)

[beauby]

@ram535ii The format would have to be as described by the JSON API spec:

The value of the include parameter MUST be a comma-separated (U+002C COMMA, ",") list of relationship paths. A relationship path is a dot-separated (U+002E FULL-STOP, ".") list of relationship names.
Example: 'posts.comments.author,comments.author'

An other option is to use the more format-permissive ActiveModel::Serializer::IncludeTree::Parsing.include_string_to_hash, to which you can pass a mix of arrays/hashes.
Example: [:posts, comments: [:author, :likes], author: { posts: [:comments] }]

[raphox]

For new versions (after #1685 Replace IncludeTree with IncludeDirective from the jsonapi gem.):

JSONAPI::IncludeDirective.new(params[:include], allow_wildcard: true).to_hash

# In: <string>"tags,author.user"
# Out: <hash> {:tags=>{}, :author=>{:user=>{}}}