Add the ability to preload with a scope

[CyborgMaster]

This allows a preload scope to be specified when preloading associations. It's based of the technique described here: http://aserafin.pl/2017/09/12/preloading-associations-with-dynamic-condition-in-rails/

The problem we were running into was when we had an association that they wanted to filter like this:

{
  allClients(first: 5){
    first_name
    transactions(filter: { posted_at_gt: "2017-10-01"}) {
      amount
    } 
  } 
}

Without this we have two options: preload normally which will load all the transactions (potentially WAY too many) and then filter them afterwords, or run an N+1 query for each client that will load the transactions for that client with the appropriate where clause. Neither are performant.

This patch will allow loading all of this with two SQL statements: one for the clients and one for the transactions.

I'm not sure about the implementation. Specifically how the scope lambda is an initialization argument that isn't used and how the scope gets overwritten each time. However due to the limitations of graphql-batch, this was the best I could come up with. I'm happy to discuss why I did it the way I did, or better ideas if you have them.

[etripier]

Hey @CyborgMaster! Thanks for the great PR!

I was thinking about how you pass the scope all the way down here, and realized that in Rails 3, ActiveRecord::Associations::Preloader#new actually takes an options hash as the third argument. It looks like if we want to preserve Rails 3 compatibility, we may have to modify the interface for this gem to accept a hash of conditions instead of a callable.

I took a shot at implementing that here: https://github.com/ConsultingMD/graphql-preload/compare/feature/preload-with-scope?expand=1. Would you mind taking a look and letting me know what you think? Does this also solve your problem (and does it present an interface you're happy with)?

[CyborgMaster]

Right, I didn't look at Rails 3 compatibility at all, so we'll have to address that somehow.

Just to clarify, in Rails 4, the third parameter isn't a callable, it takes a Relation, something like Transactions.where('posted_at > 2017-10-01').

The problem I am trying to work around is the preload scope needs both the args and context, which is why it got passed all the way down in that strange manner in my original PR.

In the example I gave above the preload scope would end up being something like this (simplified in this example to just the pertinent code):

preload_scope lambda { |args, ctx|
  Transactions
    .where(source_id: ctx[:source_id])
    .where('posted_at > ?', args[:filter][:posted_at_gt])
}

so we need the context for security purposes and the args to determine the filters applied at that level, neither of which we have if we just do an instance eval.

Thoughts?

[etripier]

I see what you mean. I was thinking about this a bit over the weekend; we could modify the interface to return the conditions expected by Rails 3:

preload_conditions ->(obj, args, ctx) do
  ['source_id = ? and posted_at > ?', ctx[:source_id], args[:filter][:posted_at_gt]]
end

Then, pass that straight to Rails 3, or turn it into a relation by calling klass.where(preload_conditions) in Rails 4. Of course, that's not a great interface, and more importantly, it sacrifices a lot of functionality (like calling a named scope). It might be easier just to cut the Rails 3 compatibility for this feature.

[CyborgMaster]

Yeah, or doing any other advanced things in, say Arel, that are cumbersome to do in String form (this is actually what we are doing in our system). I'd be fine if we just had it work for Rails 4.

[etripier]

Did you use scope_lambda to generate a more unique loader key? Is there a way to avoid passing scope_lambda all the way down here? What about using scope.to_sql (or in Rails 5, scope.cache_key) instead?

[CyborgMaster]

Yeah, we needed the loader key. The problem we needed a loader key that wouldn't change from invocation to invocation. I didn't think about using scope.to_sql, that could work.

[CyborgMaster]

Sorry finally found some time to look at this. You we're totally right. I should be using scope.to_sql, which is working great. I'm no longer passing scope_lambda all over the place. Much cleaner.

[CyborgMaster]

I've addressed the scope lambda issue. Is this enough to get this merged? (PS. I should be opening another PR shortly getting this to work for graphql 1.8)

[etripier]

scope may or may not be defined here, so we should at least wrap this in a try.

[CyborgMaster]

you are totally correct. Just ran into that myself. I'll be updating shortly.

[etripier]

@CyborgMaster Thank you very much for making that change. After you make the change to scope.to_sql, I'll merge this in, deprecate Rails 3.x support, and publish the gem update with some supporting documentation.

[CyborgMaster]

I fixed the nil scope crash. I'm also about to open a second PR to support the newest version of graphql, 1.8.0.pre11. So you may want to merge both of them at the same time.

[etripier]

Sorry, one last minor note: Would you use try instead of the & operator? I actually prefer using &, but it comes with a pretty steep Ruby 2.3 requirement (and Rails 4 only requires Ruby 2.0, I believe).

[CyborgMaster]

Done. I had used & twice and I pulled them both out.