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

GraphQL v1.8.7 not compatible with Batch Loader #22

Closed
JanStevens opened this issue Aug 15, 2018 · 8 comments · Fixed by #32
Closed

GraphQL v1.8.7 not compatible with Batch Loader #22

JanStevens opened this issue Aug 15, 2018 · 8 comments · Fixed by #32

Comments

@JanStevens
Copy link

Using Batch Loader and GraphQL v1.8.6

  Event Load (0.9ms)  SELECT "events".* FROM "events"
  ↳ app/controllers/api/graphql_controller.rb:11
  EventPeriod Load (0.9ms)  SELECT "event_periods".* FROM "event_periods" WHERE "event_periods"."event_id" IN ($1, $2, $3)  [["event_id", 1], ["event_id", 2], ["event_id", 3]]
  ↳ app/graphql/resolvers/event_periods_resolver.rb:10
  EventPeriod::Grouping Load (0.7ms)  SELECT "event_period_groupings".* FROM "event_period_groupings" WHERE "event_period_groupings"."event_period_group_id" IN ($1, $2)  [["event_period_group_id", 5], ["event_period_group_id", 12]]
  ↳ app/graphql/resolvers/event_period_days_resolver.rb:10
  EventPeriod::Day Load (0.7ms)  SELECT "event_periods".* FROM "event_periods" WHERE "event_periods"."type" IN ('day') AND "event_periods"."id" IN ($1, $2, $3, $4, $5)  [["id", 1], ["id", 2], ["id", 3], ["id", 4], ["id", 11]]
  ↳ app/graphql/resolvers/event_period_days_resolver.rb:10

Using GraphQL v1.8.7

  Event Load (0.8ms)  SELECT "events".* FROM "events"
  ↳ app/controllers/api/graphql_controller.rb:11
  EventPeriod Load (1.1ms)  SELECT "event_periods".* FROM "event_periods" WHERE "event_periods"."event_id" = $1  [["event_id", 1]]
  ↳ app/graphql/resolvers/event_periods_resolver.rb:10
  EventPeriod Load (0.6ms)  SELECT "event_periods".* FROM "event_periods" WHERE "event_periods"."event_id" = $1  [["event_id", 2]]
  ↳ app/graphql/resolvers/event_periods_resolver.rb:10
  EventPeriod Load (0.4ms)  SELECT "event_periods".* FROM "event_periods" WHERE "event_periods"."event_id" = $1  [["event_id", 3]]
  ↳ app/graphql/resolvers/event_periods_resolver.rb:10
  EventPeriod::Grouping Load (0.8ms)  SELECT "event_period_groupings".* FROM "event_period_groupings" WHERE "event_period_groupings"."event_period_group_id" = $1  [["event_period_group_id", 5]]
  ↳ app/graphql/resolvers/event_period_days_resolver.rb:10
  EventPeriod::Day Load (0.8ms)  SELECT "event_periods".* FROM "event_periods" WHERE "event_periods"."type" IN ('day') AND "event_periods"."id" IN ($1, $2, $3, $4)  [["id", 1], ["id", 2], ["id", 3], ["id", 4]]
  ↳ app/graphql/resolvers/event_period_days_resolver.rb:10
  EventPeriod::Grouping Load (0.4ms)  SELECT "event_period_groupings".* FROM "event_period_groupings" WHERE "event_period_groupings"."event_period_group_id" = $1  [["event_period_group_id", 12]]
  ↳ app/graphql/resolvers/event_period_days_resolver.rb:10
  EventPeriod::Day Load (0.4ms)  SELECT "event_periods".* FROM "event_periods" WHERE "event_periods"."type" IN ('day') AND "event_periods"."id" IN ($1, $2, $3, $4, $5)  [["id", 1], ["id", 2], ["id", 3], ["id", 4], ["id", 11]]
  ↳ app/graphql/resolvers/event_period_days_resolver.rb:10
Completed 200 OK in 248ms (Views: 0.4ms | ActiveRecord: 19.9ms)

Probably something has changed in GraphQL but not sure if BatchLoader should adapt or GraphQL has a bug. Related issue on GraphQL: rmosolgo/graphql-ruby#1778

Changelog: rmosolgo/graphql-ruby@v1.8.6...master

@cabello
Copy link

cabello commented Aug 15, 2018

I can't reproduce it, my Gemfile.lock contains:

graphql (1.8.7)
batch-loader (1.2.1)

While running a query that would potentially cause a N+1 I got only two queries as expected:

D, [2018-08-15T09:31:48.706667 #67559] DEBUG -- :   FundsTransfer Load (8.0ms)  SELECT  "funds_transfers".* FROM "funds_transfers" ORDER BY "funds_transfers"."created_at" DESC LIMIT $1 OFFSET $2  [["LIMIT", 50], ["OFFSET", 0]]
D, [2018-08-15T09:31:48.821072 #67559] DEBUG -- :   Account Load (4.8ms)  SELECT "accounts".* FROM "accounts" WHERE "accounts"."id" IN (13, 12, 9, 8, 7, 6)

Am I missing something? Can you post the code on how you are using batch loader?

@JanStevens
Copy link
Author

begin
  require "bundler/inline"
rescue LoadError => e
  $stderr.puts "Bundler version 1.10 or later is required. Please update your Bundler"
  raise e
end

gemfile(true) do
  source "https://rubygems.org"
  gem "graphql", '1.8.7'
  gem "batch-loader"
  gem 'pg'
  gem 'rails', '5.2.1'
end

require 'pg'
require 'active_record'
require 'active_support'
require 'logger'

ActiveRecord::Base.establish_connection(adapter: "postgresql", database: "hacky", host: 'localhost')
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :posts, force: true do |t|
    t.string :title
    t.timestamps(null: false)
  end

  create_table :comments, force: true do |t|
    t.string :title
    t.string :comment
    t.integer :post_id
    t.timestamps(null: false)
  end
end

class Post < ActiveRecord::Base
  has_many :comments
end

class Comment < ActiveRecord::Base
  belongs_to :post
end

# Data setup
post = Post.create!(title: 'AAAA1')
post.comments << Comment.create!(title: 'AAAA1')
post.comments << Comment.create!(title: 'AAAA2')
post.comments << Comment.create!(title: 'AAAA3')

post = Post.create!(title: 'BBBB1')
post.comments << Comment.create!(title: 'BBBB1')
post.comments << Comment.create!(title: 'BBBB2')
post.comments << Comment.create!(title: 'BBBB3')


class CommentType < GraphQL::Schema::Object
  field :id, ID, null: false
  field :title, String, null: false
end

class PostType < GraphQL::Schema::Object
  field :id, ID, null: false
  field :title, String, null: false
  field :comments, [CommentType], null: true

  def comments
    BatchLoader.for(object.id).batch(default_value: []) do |post_ids, loader|
      Comment.where(post_id: post_ids).each do |comment|
        loader.call(comment.post_id) { |memo| memo << comment }
      end
    end
  end
end

class QueryType < GraphQL::Schema::Object
  field :posts, [PostType], null: false

  def posts
    Post.all
  end
end

class Schema < GraphQL::Schema
  query QueryType
  use BatchLoader::GraphQL
end

query_string = <<~GRAPHQL
  query posts {
    posts {
      comments {
        id
        title
      }
    }
  }
GRAPHQL

context = {}
variables = {}

puts Schema.execute(query_string, context: context, variables: variables).to_json

You can change the graphql version from 1.8.6 (working) to 1.8.7 (not working).

Output for 1.8.6

D, [2018-08-15T15:44:30.794020 #54453] DEBUG -- :   Post Load (0.5ms)  SELECT "posts".* FROM "posts"
D, [2018-08-15T15:44:30.795289 #54453] DEBUG -- :   Comment Load (0.4ms)  SELECT "comments".* FROM "comments" WHERE "comments"."post_id" IN ($1, $2)  [["post_id", 1], ["post_id", 2]]
{"data":{"posts":[{"comments":[{"id":"1","title":"AAAA1"},{"id":"2","title":"AAAA2"},{"id":"3","title":"AAAA3"}]},{"comments":[{"id":"4","title":"BBBB1"},{"id":"5","title":"BBBB2"},{"id":"6","title":"BBBB3"}]}]}}

Output for 1.8.7

D, [2018-08-15T15:44:52.600755 #54659] DEBUG -- :   Post Load (0.4ms)  SELECT "posts".* FROM "posts"
D, [2018-08-15T15:44:52.601887 #54659] DEBUG -- :   Comment Load (0.2ms)  SELECT "comments".* FROM "comments" WHERE "comments"."post_id" = $1  [["post_id", 1]]
D, [2018-08-15T15:44:52.603110 #54659] DEBUG -- :   Comment Load (0.3ms)  SELECT "comments".* FROM "comments" WHERE "comments"."post_id" = $1  [["post_id", 2]]
{"data":{"posts":[{"comments":[{"id":"1","title":"AAAA1"},{"id":"2","title":"AAAA2"},{"id":"3","title":"AAAA3"}]},{"comments":[{"id":"4","title":"BBBB1"},{"id":"5","title":"BBBB2"},{"id":"6","title":"BBBB3"}]}]}}

@JanStevens
Copy link
Author

It seems that the API changed of GraphQL Ruby and basically the gist is rmosolgo/graphql-ruby#1778 (comment)

Any reason why class method is undefined?

@exAspArk
Copy link
Owner

exAspArk commented Aug 15, 2018

@JanStevens I was able to reproduce it, thanks for the code example!

That's sad that graphql-ruby introduced a breaking change and started wrapping the values with it's own lazy objects.

Any reason why class method is undefined?

It's not undefined, it's being proxied to the resolved value. For example:

id = 1

result = BatchLoader.for(id).batch { |ids, loader| loader.call(ids.first, 1) }
result.class # => Fixnum

result = BatchLoader.for(id).batch { |ids, loader| loader.call(ids.first, nil) }
result.class # => NilClass

@exAspArk
Copy link
Owner

There are some ideas on how to make graphql-ruby more flexible in terms of this "lazy" functionality. Maybe they'll be implemented one day.

In a meantime, here is a quick fix:

def comments
  batch_loader = BatchLoader.for(object.id).batch(default_value: []) do |post_ids, loader|
    Comment.where(post_id: post_ids).each do |comment|
      loader.call(comment.post_id) { |memo| memo << comment }
    end
  end

  BatchLoader::GraphQL::Wrapper.new(batch_loader) # it is done automatically with graphql-ruby 1.8.6
end

Just wrap BatchLoader instance into BatchLoader::GraphQL::Wrapper.

That way the new graphql-ruby version will be able to understand that it's a lazy object, before wrapping it into another lazy GraphQL::Execution::Lazy object.

@exAspArk
Copy link
Owner

exAspArk commented Aug 15, 2018

@rmosolgo is so crazy fast! He already started working on rmosolgo/graphql-ruby#1784 🚀

That potentially will allow DRYing the BatchLoader::GraphQL::Wrapper logic instead of repeating it everywhere. Or something similar to detect lazy BatchLoader instances in the GraphQL world. Right now it's done by using .class.

@Batou99
Copy link

Batou99 commented Sep 4, 2018

Has this been fixed on 1.8.8?

@exAspArk
Copy link
Owner

I added tests and set up a build matrix to test the latest GraphQL 1.8 and 1.7 versions in this PR #28.

it 'resolves BatchLoader fields lazily' do
user1 = User.save(id: "1")
user2 = User.save(id: "2")
Post.save(user_id: user1.id)
Post.save(user_id: user2.id)
query = <<~QUERY
{
posts {
user { id }
userId
}
}
QUERY
expect(User).to receive(:where).with(id: ["1", "2"]).once.and_call_original
result = GraphqlSchema.execute(query)
expect(result['data']).to eq({
'posts' => [
{'user' => {'id' => "1"}, "userId" => 1},
{'user' => {'id' => "2"}, "userId" => 2}
]
})
end

It seems like it works fine with graphql version 1.8.11 now? Going to close the issue. Feel free to reopen if it still doesn't work.

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

Successfully merging a pull request may close this issue.

4 participants