Graphql-ruby: GraphQL v1.8.7 break compatibility with BatchLoader

[JanStevens]

GraphQL V1.8.6 works perfectly with https://github.com/exAspArk/batch-loader after upgrading to v1.8.7 the fields are not lazy resolved, so something changed

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)

Related issue on Batch-Loader: https://github.com/exAspArk/batch-loader/issues/22

[rmosolgo]

Hi, sorry for the breakage! Could you please share the GraphQL query string and the Ruby code for these types and fields?

[JanStevens]

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"}]}]}}

[rmosolgo]

Awesome, thanks for that great repro!

[rmosolgo]

Looks like a bug in the new scoping feature, I made a change to get the proper database behavior:

-   field :comments, [CommentType], null: true
+   field :comments, [CommentType], null: true, scope: false

Now, to find the bug!

[rmosolgo]

Ok, I think I see. batch-loader checks the return type of _every_ field call to see whether it's a loader that should be wrapped:

https://github.com/exAspArk/batch-loader/blob/6730dfa775c230de5502d356ca974e486f6bdd8c/lib/batch_loader/graphql.rb#L22-L25

But, in 1.8.7, sometimes return values are wrapped with a GraphQL::Execution::Lazy, so that scope_items can be called on their eventual return value:

https://github.com/rmosolgo/graphql-ruby/blob/34c0bdbf14ed7bd6dafc321c9acd06abb7e79098/lib/graphql/schema/field.rb#L499-L509

So the problem is, in 1.8.7, fields that return a batch loader _now_ return a Lazy which wraps the batch loader, so the instrumentation doesn't properly wrap it.

🤔 Ok, now how to fix! 😆

[JanStevens]

Great! For my quick code reading scans it seems that a general concept for a pluggable Lazy loader is needed, if that is proper defined then BatchLoader can easily integrated with it (as any other batch loader)

[rmosolgo]

The good news is, _is_ a pluggable lazy loader: http://graphql-ruby.org/schema/lazy_execution.html

In fact, batch-loader uses this system under the hood:

https://github.com/exAspArk/batch-loader/blob/038b554a2498a6831aef9665ce724cda18f0d4da/lib/batch_loader/graphql.rb#L16

The problem is that GraphQL-Ruby uses an object's class to determine whether or not an object should be lazy-loaded, but BatchLoader#class does _not_ return BatchLoader, for example:

require "batch-loader"
loader = BatchLoader.for(1).batch { 1 + 1 }
# => #<BatchLoader:0x140288404245480>
loader.class
# => NilClass

To work around this, batch-loader applies a wrapper:

https://github.com/exAspArk/batch-loader/blob/038b554a2498a6831aef9665ce724cda18f0d4da/lib/batch_loader/graphql.rb#L24

But the condition for applying that wrapper no longer holds true in 1.8.7.

[rmosolgo]

The problem is, GraphQL has started _checking_ return_value.class to know whether or not it should be batched.

However, batch-loader _undefines_ the .class method and reimplements it to load the value.

So, the cause for regression is that GraphQL-Ruby calls .class on a BatchLoader instance. You can fix the repro posted above by reimplementing class:

class BatchLoader
  def class
    BatchLoader
  end
end

Unless there's a good reason that BatchLoader#class shouldn't return the object's Class, I don't think I'm going to change GraphQL-Ruby to support this. If needed, I can document the requirement that returned objects implement .class to return their Class.

Is there any reason that BatchLoader#class can't return BatchLoader?

[JanStevens]

Maybe @exAspArk has some insight on why this is undefined? I'll post an update on the BatchLoader issue https://github.com/exAspArk/batch-loader/issues/22

Thanks a lot for the investigation, I can indeed add this monkey patch for now and see how well it goes, I understand you don't want to change your API, seems to me indeed odd that class is undefined in BatchLoader

[exAspArk]

@rmosolgo hey, thanks for finding the root cause!

Is it possible to specify the order for instrumentations? For example, instrument values _before_ recursively wrapping them into GraphQL::Execution::Lazy objects (since 1.8.7)?

I imagine that it can be useful in general. For example, to measure the actual performance for resolvers which just create and return lazy objects.

[rmosolgo]

Sorry, my earlier comment about GraphQL::Execution::Lazy was mistaken!

In fact, in 1.8.7, graphql-ruby _checks_ whether the returned object is lazy, but in the case of batch-loader, it _isn't_, because the field returns a BatchLoader instance, but only BatchLoader::Wrapper is registered with graphql-ruby via lazy_resolve.

The problem is that graphql-ruby checks by calling value.class, which, for BatchLoader instances, causes the block to run.

[exAspArk]

As far as understand it's this if statement:

https://github.com/rmosolgo/graphql-ruby/blob/7715a4ad36780991561e33e026bb8c5c722fe6b9/lib/graphql/schema.rb#L1005-L1015

So lazy_method_name uses a map by using value.class as a key. However, BatchLoader by design proxies any method calls to the resolved objects including .class.

Seems like, previously, either:

• value.class was returning BatchLoader::GraphQL::Wrapper – instrumentation was called _before_. If so, is it possible to return the previous behavior?
• or after_lazy wasn't called on the value?

[rmosolgo]

Yep, spot on -- previously, .after_lazy wasn't called on the value.

[exAspArk]

@rmosolgo got it, thanks!

Just as an idea, for me, in the ideal world, the lazy functionality could look something like:

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

class Schema < GraphQL::Schema
  query QueryType

  def lazy_resolve(value)
    value.sync # or it can be a lot more than just a simple method call
  end
end

That gives a lot of flexibility and doesn't require to be constrained to use a specific class as a signature and a specific method to resolve the lazy values.

In a meantime, I'd need to find a workaround to make BatchLoader work with GraphQL::Execution::Lazy :)

[rmosolgo]

Thanks for the suggestion, that's really good! I was brainstorming other ideas over at https://github.com/rmosolgo/graphql-ruby/pull/1411#discussion_r194159423, but your suggestion here is better because it's simple, schema-local and backwards-compatible. I'll try it out soon!

By the way, another solution might be to add class to IMPLEMENTED_INSTANCE_METHODS.

[clader]

Just wanted to see if there was any movement on this. My project is currently on 1.8.5 and I use graphql-batch heavily. I’m eager to update to the newer gem versions to get recent fixes, but am afraid to break the batching functionality.

[rmosolgo]

graphql-batch isn't broken, batch_loader is broken.

[DamirSvrtan]

As much as I see a fix for this can be to note that your has many's are not scoped (if you don't need scoping like I don't)

class PostType < GraphQL::Schema::Object
  field :comments, [CommentType], null: true, scope: false

  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

[DamirSvrtan]

Is there an API that one could use to totally disable scoping?

[DamirSvrtan]

Ok, so what we did to fix this issue is the following. We upgraded from the graphql gem from 1.8.5 to 1.8.7. While upgrading, the tests have shown that the batch-loader stopped batching requests.

The reason for that is the new :scope option that broke the batch-loader. Until there is a good API to fix this, and since we don't use scoping, we have introduced a BaseField that has the scope set to false.

Adding a base field is used for providing defaults to all the fields (that are easily overridable on a per field basis). It is following recipes that can be found here:
http://graphql-ruby.org/fields/introduction.html#field-parameter-default-values
http://graphql-ruby.org/type_definitions/extensions.html#customizing-fields

module Types
  class BaseField < GraphQL::Schema::Field
    def initialize(*args, scope: false, **kwargs, &block)
      super
    end
  end
end

module Types
  class BaseObject < GraphQL::Schema::Object
    field_class BaseField
  end
end

[axelson]

It looks like this might be fixed with graphql 1.8.11: https://github.com/exAspArk/batch-loader/issues/26#issuecomment-443329962

[swalkinshaw]

Closing since this was an issue with batch_loader which was fixed in https://github.com/exAspArk/batch-loader/pull/32.