GraphQL v1.8.7 break compatibility with BatchLoader #1778
Comments
|
Hi, sorry for the breakage! Could you please share the GraphQL query string and the Ruby code for these types and fields? |
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_jsonYou can change the graphql version from Output for 1.8.6 Output for 1.8.7 |
|
Awesome, thanks for that great repro! |
|
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: falseNow, to find the bug! |
|
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: But, in 1.8.7, sometimes return values are wrapped with a graphql-ruby/lib/graphql/schema/field.rb Lines 499 to 509 in 34c0bdb So the problem is, in 1.8.7, fields that return a batch loader now return a 🤔 Ok, now how to fix! 😆 |
|
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) |
|
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: The problem is that GraphQL-Ruby uses an object's class to determine whether or not an object should be lazy-loaded, but To work around this, batch-loader applies a wrapper: But the condition for applying that wrapper no longer holds true in 1.8.7. |
|
The problem is, GraphQL has started checking However, So, the cause for regression is that GraphQL-Ruby calls class BatchLoader
def class
BatchLoader
end
endUnless there's a good reason that Is there any reason that |
|
Maybe @exAspArk has some insight on why this is undefined? I'll post an update on the BatchLoader issue exAspArk/batch-loader#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 |
|
@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 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. |
|
Sorry, my earlier comment about 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 The problem is that graphql-ruby checks by calling |
|
As far as understand it's this graphql-ruby/lib/graphql/schema.rb Lines 1005 to 1015 in 7715a4a So Seems like, previously, either:
|
|
Yep, spot on -- previously, |
|
@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
endThat 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 |
|
Thanks for the suggestion, that's really good! I was brainstorming other ideas over at #1411 (comment), 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 |
|
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. |
|
|
|
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 |
|
Is there an API that one could use to totally disable scoping? |
|
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 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: module Types
class BaseField < GraphQL::Schema::Field
def initialize(*args, scope: false, **kwargs, &block)
super
end
end
endmodule Types
class BaseObject < GraphQL::Schema::Object
field_class BaseField
end
end |
|
It looks like this might be fixed with graphql 1.8.11: exAspArk/batch-loader#26 (comment) |
|
Closing since this was an issue with |
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
Using GraphQL v1.8.7
Related issue on Batch-Loader: exAspArk/batch-loader#22
The text was updated successfully, but these errors were encountered: