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

Examples of field resolution affecting parent loader strategy #39

Open
bessey opened this issue Dec 1, 2016 · 3 comments
Open

Examples of field resolution affecting parent loader strategy #39

bessey opened this issue Dec 1, 2016 · 3 comments
Labels

Comments

@bessey
Copy link

bessey commented Dec 1, 2016

[This is not a bug, but I don't know where else to put this]

I am trying to wrap my head around what I think is a pretty common use case, and am hoping there's some prior art from others I can learn from. The high level summary is that, given the following query:

query fetchProperty {
  property(id: 1) { 
    id
    name
  }
  property(id: 2) { 
    id
    name
  }
}

I want my batch loader to generate the SQL SELECT id, name FROM properties WHERE id IN (1, 2). Now I have built a loader to generate SELECT * FROM properties WHERE id IN (1, 2) without issues, but I am struggling to understand if / how its possible to have the sub-fields of the field requested affect the loader's strategy.

Relevant code for query:

module GraphType
  QueryType = GraphQL::ObjectType.define do
    field :property, -> { PropertyType } do
      argument :id, !types.ID
      resolve -> (obj, args, ctx) {
        # this is a simple ActiveRecord batch loader that aggregates IDs and
        # fetches them with `.where(id: ids)`
        GraphTool::FindLoader.for(Property.all).load(args[:id])
      }
    end
  end
end


module GraphType
  PropertyType = GraphQL::ObjectType.define do
    field :id, !types.ID
    field :name, !types.String
    field :address, !types.String
    field :email, !types.String
  end
end

I guess what I'm asking is, is there a context available where the property promise has not yet been resolved, but property.name's resolve function is being executed?

@dylanahsmith
Copy link
Contributor

am hoping there's some prior art from others I can learn from

Sorry, this isn't something that much to us since we use IdentityCache to cache whole rows, so haven't implemented it.

I am struggling to understand if / how its possible to have the sub-fields of the field requested affect the loader's strategy.

It could be implemented by doing the loads at the field level for the specific column that you need. In this case your cache key would be an [id, column_name] pair. E.g. it could have an interface like this

ColumnLoader.for(User).load([user_id, :first_name])

Then in ColumnLoader#perform you could collect the set of columns to use in the query.

If you need a different set of columns for each record, then you get into the question of whether it is better to over-fetch by doing a single query for the superset of fields for all records, or whether you would prefer to do separate queries for each group of records that are loading the same set of columns.

@dylanahsmith
Copy link
Contributor

Another option would be to use GraphQL::Execution::Lookahead to figure out what subfields are requested to have it influence the query.

I think the simplest way to handle this would be to group by ast nodes (e.g. lookahead.ast_nodes) so that it performs a single query for that field selection for all of its parent objects. GraphQL::Batch::Loader could overridden to be able to pass in the Lookahead object but use the Lookahead#ast_nodes for the key for grouping.

The more difficult part is to determine what database columns are needed based on the GraphQL subfield selections. This is also something that is outside the scope of this library.

@robinboening
Copy link

I think this is an interesting discussion as I am as well looking for a solution to eager load records while selecting only requested columns. I believe Lookahead is the right approach to that as it serves exactly that purpose.

I understand and respect why it's outside the scope of this library to implement a proper mechanism for selecting. As you already said the Loader needs to be overwritten. I believe it'll be quite some overwrites, probably not just one. This kind of monkey patching is definitely not my preferred way of doing it and I am wondering if you're open to investigate if there is a nice and clean way for this library to provide an interface allowing to extend the related parts.

I would also love to hear if anyone else already got a clean implementation where batch loading and selecting works side by side?

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

No branches or pull requests

3 participants