Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
bgentry opened this issue Nov 4, 2017 · 3 comments
Closed

Comments

@bgentry
Copy link

bgentry commented Nov 4, 2017

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
Copy link
Author

bgentry commented Nov 4, 2017

I guess #6 is basically a solution to this?

@exAspArk
Copy link
Owner

exAspArk commented Nov 4, 2017

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
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants