Use metaprogramming

[dlanileonardo]

An Example:

meta_programming = {
  user: { name: 'User', ids: [1, 2, 3], items: [] },
  role: { name: 'Role', ids: [1, 2, 4, 5], items: [] },
}.each do |type, kind|
  kind[:ids].map do |id|
    kind[:items] << BatchLoader.for(id, type, kind).batch do |ids, loader, slug, context|
      Object.const_get(slug.capitalize).where(id: ids).each { |item| loader.call(item.id, item) }
    end
  end
end

For example in GraphQL:

associations = model.reflections
...
associations.each do |name, association|
  slug = "#{model_name}:#{name}"
  ...
  BatchLoader.for(obj.send(association.active_record_primary_key), slug).batch(cache: true) do |keys, loader, slug_return|
    association = storage[slug_return]
    foreign_key = association.foreign_key
    association.klass.where({foreign_key => keys}).each { |item| loader.call(item.send(foreign_key), item) }
  end
end

The secret is here, because block.source_location doesn’t work well when using metaprogramming

@block_hash_key = slug || block.source_location

[coveralls]

Changes Unknown when pulling 9d469489a710f2867b56b6dc789ccbdecd9e26a1 on dlanileonardo:master into on exAspArk:master.

[coveralls]

Changes Unknown when pulling 4a6dd7088d92ab122a7587ae7497d41fba62d402 on dlanileonardo:master into on exAspArk:master.

[bessey]

Just want to say 👍 this was next on my wish list for this gem!

[exAspArk]

Nice! I was also thinking about different solutions to allow using dynamically defined BatchLoader.

One of the approaches is to use source code analyzers like ruby2ruby or sourcify: hash(block.to_source) when there is no correct block.source_location. But it's an extra dependency + these tools are probably not so performant.

Allowing to specify the block id is a great idea! I'll review the source code shortly.

[coveralls]

Coverage increased (+0.03%) to 99.736% when pulling 9bbbd779fabfdd110ab7bb88ecffc685ac45b348 on dlanileonardo:master into a5293963b221e0725ec4d1d284b9aabcb2d8ef0e on exAspArk:master.

[coveralls]

Coverage increased (+0.03%) to 99.737% when pulling f4ef7792fa527e8afafb203fb255d1722e1adca2 on dlanileonardo:master into a5293963b221e0725ec4d1d284b9aabcb2d8ef0e on exAspArk:master.

[coveralls]

Coverage increased (+0.03%) to 99.74% when pulling 6828f68701f41a60723bfa22f81e0e85012cc912 on dlanileonardo:master into a5293963b221e0725ec4d1d284b9aabcb2d8ef0e on exAspArk:master.

[dlanileonardo]

Hey @exAspArk and @bessey, I did the changes according to suggestions, follow bellow an exemple:

kind[:items] << BatchLoader.for(id).batch(key: type, context: { status: "active" }) do |ids, loader, key, contexts|
  first_key, first_value = contexts.first
  where = {id: ids}.merge(first_value)
  Object.const_get(key.capitalize).where(where).each { |item| loader.call(item.id, item) }
end

However you can use too the parameter “contexts” with more complexity if you need, because its return a Hash of values by ID key. 😄

[coveralls]

Coverage increased (+0.03%) to 99.74% when pulling 9c4db95b55279002ded7041aa4d386b264bf4527 on dlanileonardo:master into a5293963b221e0725ec4d1d284b9aabcb2d8ef0e on exAspArk:master.

[exAspArk]

Thanks, @dlanileonardo!

I was thinking about simplifying the PR as much as possible. So, at this point, can we just add an optional key argument?

BatchLoader.for(id).batch(key: "mykey") do |ids, loader|
  ...
end

Just with these changes your code in the specs can be rewritten, so you can accomplish the same, for example:

...
kind[:ids].map do |id|
  items[type] << BatchLoader.for(id).batch(key: type) do |ids, loader|
    # expect(kind.keys).to eq(ids)
    # expect(kind.map{|k,v|v[:name]}).to eq(Array.new(ids.size, kind[:name]))
    Object.const_get(type.capitalize).where(id: ids).each { |item| loader.call(item.id, item) }
  end
end
...

Overall looks great! 😻

[bessey]

As an aside, I think I understand why you feel the need to add the context argument (you're not passing a unique identifier as the key?). If you move the responsibility of ensuring key is unique, context is redundant.

For example, in my graphql-batch-loader loaders, I use the sql query itself as the key by calling .to_sql on my relation. If you did this (or the equivalent in your orm of choice) you wouldn't need key and context.

On Tue, 7 Nov 2017, 09:00 exAspArk, notifications@github.com wrote:

Thanks, @dlanileonardo https://github.com/dlanileonardo!

I was thinking about simplifying the PR as much as possible. So, at this point, can we just add an optional key argument?

BatchLoader.for(id).batch(key: "mykey") do |ids, loader| ...end

Just with these changes your code in the specs can be rewritten, so you can accomplish the same, for example:

... kind[:ids].map do |id| items[type] << BatchLoader.for(id).batch(key: type) do |ids, loader|

expect(kind.keys).to eq(ids)

# expect(kind.map{|k,v|v[:name]}).to eq(Array.new(ids.size, kind[:name]))
Object.const_get(type.capitalize).where(id: ids).each { |item| loader.call(item.id, item) }

endend ...

Overall looks great! 😻

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/exAspArk/batch-loader/pull/6#issuecomment-342549921, or mute the thread https://github.com/notifications/unsubscribe-auth/AArOaOUT41CAj7ruLiVxsBIXDJ5AMtyRks5s0Iy4gaJpZM4QJWPF .

[dlanileonardo]

I will try explain, with a more complexy example:

I have graphql in my backend, with metaprogramming for mount Types which abstract ActiveRecord, in this case I have an association has_many with multiple keys to foreign_key: id, period_id and customer_id. (In Ideal world ActiveRecord works perfectly but in real world ActiveRecord is Slow and we need more for improve performance of querys.)

POST /api/graphql/query.json

{
  customer {
    periods(limit: 2){
      id
      entities(order_by: "name"){
        name
        team(order_by: "name"){
          name
        }
      }
    }
  }
}

In my Backend

...
def self.phrogz2a(hashes)
  {}.tap{ |r| hashes.each{ |h| h.each{ |k,v| (r[k]||=[]) << v } } }
end

...
fixed_where = DynamicType.fixed_where(fields, obj) # here I got customer_id and period_id in 2 objects
context = {fixed_where: fixed_where, args: args}

# Think in moment that Graphql will try get team or entities of query.
BatchLoader.for(obj.send(association.active_record_primary_key)).batch(cache: true, key: slug, context: context) do |keys, loader, slug_return, context_return|
  association = storage[slug_return]
  fixed_where = DynamicType.phrogz2a(context_return.map { |y,x| x[:fixed_where] }) # Here context get an array with keys from obj1 and obj2 or period 1 and period 2 OR  entity1, entity2 ...
  args = context_return[keys.first][:args] # I need just one argument because this is redundant
  permited_fields = DynamicType.permited_fields(args.to_h, association.klass.columns.map(&:name))
  foreign_key = association.foreign_key

  search = association.klass.where({foreign_key => keys}).where(fixed_where).where(permited_fields).order(args[:order_by]) 

  search.group_by { |i| i.send(foreign_key) }.each { |key, items|
    loader.call(key, items.take(args[:limit]))
  }
end
...

I can create an array fixed_where and push inside, but it will create more complexity in my code and BatchLoader can control this with more simplicity and safe way.

Please let me know what you think about this. 😁

[exAspArk]

To be honest, I don't quite understand everything in the source code :) Is it executed at the GraphQL field level?

Since you define slug / context variables outside BatchLoader, is it possible to use them inside?

slug = "#{model_name}:#{name}"
...
context = {fixed_where: fixed_where, args: args}

BatchLoader.for(obj.send(association.active_record_primary_key)).batch(key: slug) do |keys, loader|
  association = storage[slug]
  fixed_where = DynamicType.phrogz2a(context.map { |y,x| x[:fixed_where] }) 
  ...
end

[exAspArk]

I think that adding a context might be a good idea but IMHO it's better not to do that in the PR.

If it's really necessary to share the data between BatchLoader instances based on the key, for now, it's possible to do something like:

global_context = Hash.new { |hash, key| hash[key] = {} } # can be a GraphQL context

item_by_class.each do |klass, item|
  BatchLoader.for(item).batch(key: klass) do |items, loader|
    context = global_context[klass]
    ...
  end
end

[exAspArk]

Hey @dlanileonardo and @bessey!

I opened the #12 PR on top of this one. I took the existing commits as it is (just squashed them into one) and added one extra commit. There I removed the context and added some description to README.

Instead of context, I think it can be possible to pass the data as a key if necessary. For example:

key = {type: User, context: {status: 'active'}}

BatchLoader.for(id).batch(key: key) do |ids, loader, args|
  model = args.dig(:key, :type)
  context = args.dig(:key, :context)

  model.where(context).where(id: ids).each { |record| loader.call(record.id, record) }
end

Could you guys please review it and let me know what you think?

[dlanileonardo]

Its a good idea, I like this way.

😁