Skip to content

Help with resolvers #166

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

Closed
bmarini opened this issue Jun 10, 2016 · 10 comments
Closed

Help with resolvers #166

bmarini opened this issue Jun 10, 2016 · 10 comments

Comments

@bmarini
Copy link

bmarini commented Jun 10, 2016

class Chat < ActiveRecord::Base
  has_many :chat_messages
end

class ChatMessage < ActiveRecord::Base
  belongs_to :chat
end

ChatType = GraphQL::Object.define do
  field :id, !types.ID, "The ID of the chat."
  field :creator_id, !types.ID, "The ID of the creator of this chat."
  field :title, !types.String, "The title of this chat."
  field :chat_messages, ChatMessageType, "All the chat messages for this chat."
end

ChatMessageType = GraphQL::Object.define do
  field :id, !types.ID, "The ID of this chat message."
  field :body, !types.String, "The chat message body."
end

QueryRoot = Graph::Object.define do
  field :chat do
    type ChatType
    argument :ids, !types[types.ID], "An array of chat ids to look up"
    resolve -> (obj, args, ctx) {
      Chat.where(id: args[:ids]).includes(:chat_messages)
    }
  end
end

Schema = GraphQL::Schema.new(query: QueryRoot)

# Example 1 (Fetch all)
Schema.execute('query { chat(ids: [1,2,3]) { creator_id title chat_messages { id body } } }', context: {}, debug: true)

# Example 2 (Fetch top level chat objects only)
Schema.execute('query { chat(ids: [1,2,3]) { creator_id title } }', context: {}, debug: true)

# Example 3 (Fetch subset of fields from top level object)
Schema.execute('query { chat(ids: [1,2,3]) { id title } }', context: {}, debug: true)

Given the examples above, how would I optimize my resolver for all three? For example 2, I don't want to load the chat messages. For example 3, it would be nice to only select the fields I need from the database. Should I be introspecting the query from the resolver block to figure out what to do here?

@rmosolgo
Copy link
Owner

rmosolgo commented Jun 10, 2016

✋ I don't have a really good answer 😞

The almost-answer is to inspect the query AST to see if chat messages were requested:

resolve -> (obj, args, ctx) {
  chats = Chat.where(id: args[:ids])
  if ctx.ast_node.selections.any? { |selection| selection.name == "chat_messages"} 
   chats = chats.includes(:chat_messages)
  end 
  chats
}

However, this doesn't work if fields aren't requested inline:

query {
  chat(ids: [1,2]) { ... chatFields }
}

fragment chatFields on Chat {
  chat_messages 
}

Now, the AST is all wonky: the selections include chatFields, but the fragment definition is way somewhere else! (Technically, you could probably still get to it through ctx.query.document but that's probably more trouble than it's worth, and brittle too.)

This could probably be improved by some pre-processing or a legitimate API to expose what fields will be requested from an object. But it wouldn't work for nested selections because of Unions: you can't say whether a ... on X will apply until you get the underlying object and test whether it is of type X or not!

@bmarini
Copy link
Author

bmarini commented Jun 11, 2016

Thanks for the info @rmosolgo! Is the AST wonkiness a deficiency of the ruby implementation? I'm wondering how how to solve this in graphql-ruby but also in general. If there is a best practice here, and graphql-ruby is just lacking, I'd love to pitch in and help.

@rmosolgo
Copy link
Owner

I've tried a couple things over the last year without any luck, but I can point to some other approaches:

By the way, there's also graphql-batch for Ruby which handles some efficiency issues. But I'd love to see some improvements in this area!

@chanks
Copy link
Contributor

chanks commented Jun 28, 2016

We use this gem in conjunction with Sequel, and it includes a couple of plugins that are handling these issues quite nicely for us.

For eager-loading associations, the tactical_eager_loading plugin handles them pretty seamlessly as they're accessed: https://github.com/jeremyevans/sequel/blob/master/lib/sequel/plugins/tactical_eager_loading.rb

For selecting specific fields, there isn't quite as clean a solution, but we do use the lazy_attributes plugin to ensure that some fields (large text columns, for example) are lazy-loaded from the DB only when accessed: https://github.com/jeremyevans/sequel/blob/master/lib/sequel/plugins/lazy_attributes.rb

It's possible that extending ActiveRecord to support things like this may be an easier task than manually walking the AST.

@rmosolgo
Copy link
Owner

0.16.0 introduces ctx.irep_node. You can inspect the node's children to see all fields which were queried and what types they'll apply to:

ctx.irep_node.children.each do |irep_child|
  irep_child.name # alias or field name 
  irep_child.definition # => <#GraphQL::Field ...> Field definition 
  irep_child.on_types # => Set<types...> Type definitions which this field will apply to
  irep_child.ast_node # => <#GraphQL::Language::Nodes::Field ... >
end 

I haven't had a ton of time to play around with it, but this is the data structure used for query execution now, I think it will provide better support for look-ahead too!

@ddgromit
Copy link
Contributor

I tried the AST method above but the problem is the includes doesn't actually save any DB queries because the call to the child relation uses offset, invalidating the results cached by includes. Using includes actually adds a DB query.

Item Load   SELECT "items".* FROM "items" WHERE "items"."team_id" = $1 OFFSET $2  [["team_id", 16], ["OFFSET", 0]]
ItemTag Load   SELECT "item_tags".* FROM "item_tags" WHERE "item_tags"."item_id" IN (27, 25, 26, 24, 23, 19)

####  .includes(:tags) added this
Tag Load   SELECT "tags".* FROM "tags" WHERE "tags"."id" IN (1591, 1589, 4659, 4616, 1533, 363, 1)  

### this happens with both queries
Tag Load   SELECT "tags".* FROM "tags" INNER JOIN "item_tags" ON "tags"."id" = "item_tags"."tag_id" WHERE "item_tags"."item_id" = $1 OFFSET $2  [["item_id", 27], ["OFFSET", 0]]
Tag Load   SELECT "tags".* FROM "tags" INNER JOIN "item_tags" ON "tags"."id" = "item_tags"."tag_id" WHERE "item_tags"."item_id" = $1 OFFSET $2  [["item_id", 25], ["OFFSET", 0]]
Tag Load   SELECT "tags".* FROM "tags" INNER JOIN "item_tags" ON "tags"."id" = "item_tags"."tag_id" WHERE "item_tags"."item_id" = $1 OFFSET $2  [["item_id", 26], ["OFFSET", 0]]
Tag Load   SELECT "tags".* FROM "tags" INNER JOIN "item_tags" ON "tags"."id" = "item_tags"."tag_id" WHERE "item_tags"."item_id" = $1 OFFSET $2  [["item_id", 24], ["OFFSET", 0]]
Tag Load   SELECT "tags".* FROM "tags" INNER JOIN "item_tags" ON "tags"."id" = "item_tags"."tag_id" WHERE "item_tags"."item_id" = $1 OFFSET $2  [["item_id", 23], ["OFFSET", 0]]
Tag Load   SELECT "tags".* FROM "tags" INNER JOIN "item_tags" ON "tags"."id" = "item_tags"."tag_id" WHERE "item_tags"."item_id" = $1 OFFSET $2  [["item_id", 19], ["OFFSET", 0]]

Is there any way to disable offset when offset is 0?

@rmosolgo
Copy link
Owner

Sorry to be so late on this.

Here's where offset is applied, perhaps you could skip it in case the offset is 0:

https://github.com/rmosolgo/graphql-ruby/blob/master/lib/graphql/relay/relation_connection.rb#L30

But ... I'm not sure how we'd test it :S

@ddgromit
Copy link
Contributor

Not sure how to test it, but omitting .offset when its zero seems like the way to go. ActiveRecord should know they are the same query but it doesn't. In Postgres and MySQL they are explicitly equivalent:

OFFSET 0 is the same as omitting the OFFSET clause.

https://www.postgresql.org/docs/9.0/static/queries-limit.html

The offset of the initial row is 0 (not 1)

https://dev.mysql.com/doc/refman/8.0/en/select.html

@rmosolgo rmosolgo added the bug label Dec 11, 2016
@rmosolgo
Copy link
Owner

Let's say we skipped the call to .offset when offset was zero, something like this:

https://github.com/rmosolgo/graphql-ruby/compare/skip-offset-0

Would that actually be an improvement? When someone requested "page two", then offset would still be applied. Would cause the same issue described above?

Also, GraphQL::Pro includes an alternate cursor implementation which uses column values as cursors instead of offsets, so you might check out that solution :P

@rmosolgo
Copy link
Owner

rmosolgo commented Jan 16, 2017

I guess GraphQL::Batch can be used as an approach that isn't subject to the includes issue. I hope that works for you!

If not, feel free to reopen this issue or open a new one with any details.

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

4 participants