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

Best Practice Using Batch loader on Graphql 1.8.11 #30

Closed
im-not-a-robot opened this issue Dec 26, 2018 · 14 comments
Closed

Best Practice Using Batch loader on Graphql 1.8.11 #30

im-not-a-robot opened this issue Dec 26, 2018 · 14 comments

Comments

@im-not-a-robot
Copy link

im-not-a-robot commented Dec 26, 2018

My code for batch loader :

BatchLoader.for(object.id).batch(default_value: []) do |province_ids, loader|
    City.where(province_id: province_ids).each do |city|
         loader.call(city.province_id) { |data| data << city }
     end
end

Result :

Province Load (0.7ms)  SELECT "provinces".* FROM "provinces"
↳ app/controllers/graphql_controller.rb:11
City Load (0.5ms)  SELECT "cities".* FROM "cities" WHERE "cities"."province_id" = $1  [["province_id", 1]]
↳ app/graphql/types/province_type.rb:21
 City Load (1.1ms)  SELECT "cities".* FROM "cities" WHERE "cities"."province_id" = $1  [["province_id", 2]]
↳ app/graphql/types/province_type.rb:21
City Load (0.5ms)  SELECT "cities".* FROM "cities" WHERE "cities"."province_id" = $1  [["province_id", 3]]

Here's my gemfile :

source 'https://rubygems.org'
git_source(:github) { |repo| "https://github.com/#{repo}.git" }

ruby '2.4.5'

# Bundle edge Rails instead: gem 'rails', github: 'rails/rails'
gem 'rails', '~> 5.2.1'
# Use sqlite3 as the database for Active Record
# gem 'sqlite3'
# Use postgresql as the database for Active Record
gem 'pg'
# Use Puma as the app server
gem 'puma', '~> 3.11'
# Build JSON APIs with ease. Read more: https://github.com/rails/jbuilder
# gem 'jbuilder', '~> 2.5'
# Use Redis adapter to run Action Cable in production
# gem 'redis', '~> 4.0'
# Use ActiveModel has_secure_password
# gem 'bcrypt', '~> 3.1.7'

# Use ActiveStorage variant
# gem 'mini_magick', '~> 4.8'

# Use Capistrano for deployment
# gem 'capistrano-rails', group: :development

# Reduces boot times through caching; required in config/boot.rb
gem 'bootsnap', '>= 1.1.0', require: false

# Use Rack CORS for handling Cross-Origin Resource Sharing (CORS), making cross-origin AJAX possible
# gem 'rack-cors'

group :development, :test do
  # Call 'byebug' anywhere in the code to stop execution and get a debugger console
  gem 'byebug', platforms: [:mri, :mingw, :x64_mingw]
end

group :development do
  gem 'listen', '>= 3.0.5', '< 3.2'
  # Spring speeds up development by keeping your application running in the background. Read more: https://github.com/rails/spring
  gem 'spring'
  gem 'spring-watcher-listen', '~> 2.0.0'
end


# Windows does not include zoneinfo files, so bundle the tzinfo-data gem
gem 'tzinfo-data', platforms: [:mingw, :mswin, :x64_mingw, :jruby]
gem 'graphql'

gem 'batch-loader'

Im already use your quick fix on this comment
#22 (comment)
It can solved this problem.

The other solution :
rmosolgo/graphql-ruby#1778 (comment)
But it must make scope attribute to false

Is there best practice to solve this problem ?
Thank you

@im-not-a-robot im-not-a-robot changed the title Batch loader not working on graphql 1.8.11 Best Practice Using Batch loader on Graphql 1.8.11 Dec 26, 2018
@exAspArk
Copy link
Owner

Hi @im-not-a-robot!

Did you add use BatchLoader::GraphQL to your schema?

It should work without scope: false and any manual wrapping #22 (comment). Here is the test from the current master, which uses version 1.8.11 in the build:

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

@im-not-a-robot
Copy link
Author

im-not-a-robot commented Dec 27, 2018

Yes im already add use BatchLoader::GraphQLto my schema.
I dont know why when im using make new rails project with new ruby version (2.4.5) and new graphql version (1.8.11), batch loader not work without scope: false or manual wrapping.

May if you want to see the issue, i can send you the project.

@exAspArk
Copy link
Owner

@im-not-a-robot yes, please 🙏 If you can send me the project (e.g. a github repo), that'll save me some time.

@im-not-a-robot
Copy link
Author

@FrancisMurilloDigix
Copy link

Hello,

Currently having the same problem above and the scope: false fixed my issue as well.

@ghost
Copy link

ghost commented Jan 17, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@ghost ghost added the stale Inactive label Jan 17, 2019
@im-not-a-robot
Copy link
Author

Hello @exAspArk
Have you try that project?

@ghost ghost removed the stale Inactive label Jan 17, 2019
@exAspArk
Copy link
Owner

Looking into it now

@exAspArk
Copy link
Owner

exAspArk commented Jan 25, 2019

@im-not-a-robot unfortunately, I couldn't run your rails example app. Something is wrong with a DB: partially pg, partially sqlite, no migrations, no seed data, etc.

I created a new very basic Rails app which should be relatively simple to run https://github.com/exAspArk/batch-loader-graphql. I use graphql ruby gem version 1.8.13 with the latest batch-loader version 1.2.2, and I can't reproduce it:

image

Please let me know if you can reproduce it with that rails app. Maybe the latest graphql ruby gem versions fixed compatibility again.

@im-not-a-robot
Copy link
Author

im-not-a-robot commented Jan 28, 2019

@im-not-a-robot unfortunately, I couldn't run your rails example app. Something is wrong with a DB: partially pg, partially sqlite, no migrations, no seed data, etc.

I created a new very basic Rails app which should be relatively simple to run https://github.com/exAspArk/batch-loader-graphql. I use graphql ruby gem version 1.8.13 with the latest batch-loader version 1.2.2, and I can't reproduce it:

image

Please let me know if you can reproduce it with that rails app. Maybe the latest graphql ruby gem versions fixed compatibility again.

@exAspArk Sorry for the problem. I'm already update the project, please check it again. Thank you.
Note:

  1. Im using postgresql with default username password
  2. Im already add some data to database from seeds file
  3. Im already update gemfile

@exAspArk
Copy link
Owner

exAspArk commented Jan 30, 2019

@im-not-a-robot awesome, thank you!

Could you please try these changes:

# Gemfile
gem 'batch-loader', github: 'exAspArk/batch-loader', branch: 'graphql'

You'd need to replace BatchLoader.for with BatchLoader::GraphQL.for to make it actually batch requests with the latest graphql versions. For example:

class PostType < GraphQL::Schema::Object
  field :user, UserType, null: false

  def user
    BatchLoader::GraphQL.for(object.user_id).batch do |user_ids, loader|
      User.where(id: user_ids).each { |user| loader.call(user.id, user) }
    end
  end
end

I will write more details on the issue later.

@im-not-a-robot
Copy link
Author

im-not-a-robot commented Jan 31, 2019

@im-not-a-robot awesome, thank you!

Could you please try these changes https://github.com/exAspArk/batch-loader/compare/graphql?expand=1:

# Gemfile
gem 'batch-loader', github: 'exAspArk/batch-loader', branch: 'graphql'

You'd need to replace BatchLoader.for with BatchLoader::GraphQL.for to make it actually batch requests with the latest graphql versions. For example:

class PostType < GraphQL::Schema::Object
  field :user, UserType, null: false

  def user
    BatchLoader::GraphQL.for(object.user_id).batch do |user_ids, loader|
      User.where(id: user_ids).each { |user| loader.call(user.id, user) }
    end
  end
end

I will write more details on the issue later.

Yeah its works well. Thank you !

@exAspArk
Copy link
Owner

@im-not-a-robot perfect, thank you!

I wrote an explanation about the root cause in #32. Will try to ship it tomorrow.

@exAspArk
Copy link
Owner

exAspArk commented Feb 1, 2019

Released these changes in v1.3.0:

TL;DR

  • If you use graphql gem version 1.8.6 or lower, you can continue using BatchLoader.
  • If you use graphql gem version 1.8.7 or greater, you need to replace BatchLoader.for with BatchLoader::GraphQL.for to make it work within your GraphQL resolvers (methods).

@exAspArk exAspArk closed this as completed Feb 1, 2019
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

3 participants