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

Document how one could preload has_many associations #24

Closed
cjoudrey opened this issue Oct 21, 2016 · 20 comments
Closed

Document how one could preload has_many associations #24

cjoudrey opened this issue Oct 21, 2016 · 20 comments

Comments

@cjoudrey
Copy link
Contributor

There was some discussion about how one might preload has_many associations in #graphql.

It might be interesting to share some of our AssociationLoader logic or writing a quick tutorial to achieve this in the README.

@dylanahsmith
Copy link
Contributor

I think we should make our DerivedKeyLoader builtin functionality, since is something we use to properly implement this in Shopify

@nickpoorman
Copy link

Has anyone written this up? Would love to see how you did it.

@sjchmiela
Copy link
Contributor

I created for my own purposes an ArrayLoader, which you use providing a value and a key of a has-many association, like this:

ArrayLoader.for(Comment).load([article.id, :article_id])

class ArrayLoader < GraphQL::Batch::Loader
  def initialize(model, default = nil)
    @model = model
    @default = default
  end

  def perform(keys)
    queries = {}
    keys.each do |key|
      if key.is_a?(Array)
        query_value, query_key = key
        queries[query_key] = [] if queries[query_key].nil?
        queries[query_key] << query_value
      end
    end

    results_cache = {}

    queries.each do |key, values|
      @model.where(key => values).each { |record|
        results_cache[[record[key], key]] = [] if results_cache[[record[key], key]].nil?
        results_cache[[record[key], key]] << record
      }
    end

    results_cache.each do |key, values|
      fulfill(key, values)
    end

    keys.each { |key| fulfill(key, @default || []) unless fulfilled?(key) }
  end
end

@cainlevy
Copy link

cainlevy commented Feb 3, 2017

This is what I've hit on:

class AssociationLoader < GraphQL::Batch::Loader
  def initialize(model, association)
    @model = model
    @association = association
  end

  def perform(vals)
    ActiveRecord::Associations::Preloader.new.preload(vals, @association)
    vals.each { |val| fulfill(val, val.send(@association)) }
  end
end

Used like so:

AuthorType = GraphQL::ObjectType.define do
  field :books, types[!BookType] do
    resolve ->(obj, _, _) {
      AssociationLoader.for(Author, :books).load(obj)
    }
  end
end

BookType = GraphQL::ObjectType.define do
  field :author, types[!AuthorType] do
    resolve ->(obj, _, _) {
      AssociationLoader.for(Book, :author).load(obj)
    }
  end
end

@dylanahsmith
Copy link
Contributor

Here is what we are using, which is a bit longer to:

  • give better errors if the association doesn't exist
  • call callbacks right away if the association is already loaded, potentially allows more queries to be batched
  • make sure records aren't left with an unloaded associations if there is another record in the batch load with the same id
class AssociationLoader < GraphQL::Batch::Loader
  def self.validate(model, association_name)
    new(model, association_name)
    nil
  end

  def initialize(model, association_name)
    @model = model
    @association_name = association_name
    validate
  end

  def load(record)
    raise TypeError, "#{@model} loader can't load association for #{record.class}" unless record.is_a?(@model)
    return Promise.resolve(read_association(record)) if association_loaded?(record)
    super
  end

  # We want to load the associations on all records, even if they have the same id
  def cache_key(record)
    record.object_id
  end

  def perform(records)
    preload_association(records)
    records.each { |record| fulfill(record, read_association(record)) }
  end

  private

  def validate
    unless @model.reflect_on_association(@association_name)
      raise ArgumentError, "No association #{@association_name} on #{@model}"
    end
  end

  def preload_association(records)
    ::ActiveRecord::Associations::Preloader.new.preload(records, @association_name)
  end

  def read_association(record)
    record.public_send(@association_name)
  end

  def association_loaded?(record)
    record.association(@association_name).loaded?
  end
end

@cainlevy
Copy link

cainlevy commented Feb 3, 2017

something along these lines would be a wonderful addition to either the documentation or stdlib of graphql-batch.

@theorygeek
Copy link

theorygeek commented Apr 26, 2017

If it helps, I wrote up a gist that shows how you could combine a GraphQL::Batch::Loader together with some instrumentation on your fields, in order to give you this nice syntax:

field :comments, !types[!CommentType] do
  preload comments: :author
  resolve -> (post, args, ctx) { post.comments }
end

It looks a little bit nicer than stacking a bunch of AssociationLoader's together in your resolve procs, and is pretty similar to the includes(comments: :author) syntax that Rails gives you.

https://gist.github.com/theorygeek/a1a59a2bf9c59e4b3706ac68d12c8434

@eshaiju
Copy link

eshaiju commented May 25, 2017

@theorygeek above gist worked wonderfully for me. Is this merged into gem?

@jamesbrooks
Copy link

@theorygeek that gist was also exactly what I was looking for, thanks! 👍

It would certainly be nice to have this merged into the gem, or at least some documentation written up for how to best batch association loading (I came here after spelunking into the tests for this gem to see how the test suite was doing it).

@timscott
Copy link

timscott commented Jul 7, 2017

@theorygeek - Thanks!!! I spent most of the day struggling with my crappy code. Finally I ripped out may crappy code and replaced if with your awesome code.

If your code is too specific for this gem, I suggest a new gem, call it active-graphql-batch or graphql-batch-associations.

@etripier
Copy link

Hey everyone - I took @theorygeek's excellent work and turned it into a gem. Hopefully you find it useful!

@timscott
Copy link

@etripier - Awesome!

@theorygeek
Copy link

@etripier Nice work! 🎉

@minardimedia
Copy link

@timscott @theorygeek Awesome just what I was looking for 🤗 👏🏻 this should be added to the GEM

@riffraff
Copy link

@dylanahsmith's snippet also handled the cache id smartly, IIUC, isn't that needed in the gem?

@etripier
Copy link

etripier commented Aug 1, 2017

@riffraff Good point! I went ahead and fixed that in the latest release.

@Yoshiji
Copy link

Yoshiji commented Oct 5, 2017

Hey guys, this might seem very dumb but ...

  • Why bother validating something that will anyway trigger an error if not valid (talking about the validation on the association existence)? If I remove the validate from the class, it will raise the regular ActiveRecord::AssociationNotFoundError.
  • Why bother giving the model as an argument to the constructor? If I give the wrong model class, it won't raise an error and behave like expected (assuming that you removed the validation).

The following extra light class seems to be working:

# implementation
class AssociationLoader < GraphQL::Batch::Loader
  def initialize(association_name)
    @association_name = association_name
  end

  private

  def perform(records)
    ::ActiveRecord::Associations::Preloader.new.preload(records, @association_name)
    records.each { |record| fulfill(record, record.public_send(@association_name)) }
  end
end

# usage
field :posts, !types[Types::Post] do
  resolve -> (record, _args, _ctx) { AssociationLoader.for(:posts).load(record) }
end

Am I missing something? (I am very new to GraphQL)


Following @dylanahsmith 's class in comparison to the above class:

  • give better errors if the association doesn't exist => usual ActiveRecord::AssociationNotFoundError is raised
  • call callbacks right away if the association is already loaded, potentially allows more queries to be batched => true, could easily be implemented though
  • make sure records aren't left with an unloaded associations if there is another record in the batch load with the same id => rewriting the cache_key(record) as you did could be a solution

@cainlevy
Copy link

cainlevy commented Oct 5, 2017

Sounds like the questions you're asking lead naturally to the expanded version but with stylistic variations. What motivates the analysis?

@Yoshiji
Copy link

Yoshiji commented Oct 6, 2017

I was wondering if my code was breaking something I could not "see" from my basic examples/tests.

@dylanahsmith
Copy link
Contributor

Why bother validating something that will anyway trigger an error if not valid (talking about the validation on the association existence)?

Maybe you are right. I think that validate class method might only matter if you want to use it to validate field preloads when declaring them at initialization time for fields, like in https://github.com/xuorig/graphql-active_record_batcher.

Why bother giving the model as an argument to the constructor?

The model is used for grouping so that you don't end up grouping together loads for the same association name on different models.

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