exAspArk / batch-loader

:zap: Powerful tool for avoiding N+1 DB or HTTP queries
https://engineering.universe.com/batching-a-powerful-way-to-solve-n-1-queries-every-rubyist-should-know-24e20c6e7b94
MIT License
1.04k stars 52 forks source link

Use of source_location as hash key prevents abstracting BatchLoader calls #9

Closed bgentry closed 6 years ago

bgentry commented 7 years ago

I tried to abstract out my BatchLoader calls a bit to DRY them up. In my GraphQL resolvers, I started using these field declarations:

  field :org, !Types::Org, resolve: BatchLoadResolver.new(Org)
  field :user, !Types::UserType, resolve: BatchLoadResolver.new(User)

With a BatchLoadResolver that would do the common work of loading a many:1 association:

class BatchLoadResolver
  attr_reader :id_method, :klass

  def initialize(klass, id_method = nil)
    @klass = klass
    @id_method = id_method || :"#{klass.name.underscore}_id"
  end

  def call(parent, args, ctx)
    BatchLoader.for(parent.public_send(id_method)).batch(cache: false) do |ids, loader|
      klass.where(id: ids, org: parent.org).all.each do |child|
        loader.call(child.id, child)
      end
    end
  end
end

This same loading code was working fine when I had the BatchLoader blocks defined individually for each field. However, when doing the above, all of my objects' IDs would get mixed together into a single batch (rather than a batch for each type of object).

It quickly became obvious that with a simple call like BatchLoader.for(object_id) there was clearly no way for this gem to distinguish between types of objects it would have to load. After some digging, I found the implementation call to source_location, which sets up a hash key for loaders based on where the loading block was defined. That makes sense given the minimal info I'm passing BatchLoader when executing it, but it does limit the ways this library can be used.

You may not consider this a bug, but I think you might at least want to make this more obvious in the readme. I suspect others will run into the same situation.

If this is something you care to support, maybe there could be another way to choose a hash key.

bgentry commented 7 years ago

I guess #6 is basically a solution to this?

exAspArk commented 7 years ago

Hey, @bgentry!

Thanks for opening the issue. Yes, ideally, #6 will allow specifying custom keys. With your example it may look something like:

BatchLoader.for(...).batch(key: @klass.to_s, cache: false) do |ids, loader|
  ...
end
exAspArk commented 6 years ago

We added a support for passing a key argument in the 1.2.0 version (#12) 🎉

BatchLoader.for(1).batch(key: klass) do |ids, loader|
  klass.where(id: ids, org: parent.org).all.each do |child|
    loader.call(child.id, child)
  end
end