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

Make BatchLoader work with the latest GraphQL versions #32

Merged
merged 1 commit into from
Feb 1, 2019
Merged

Conversation

exAspArk
Copy link
Owner

@exAspArk exAspArk commented Jan 31, 2019

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).

Since version v1.0.0, BatchLoader behaves as a lazy object. It only starts batching when it needs to resolve the values:

batch_loader = BatchLoader.for(1).batch(default_value: :foo) { puts "batching..." }
# => #<BatchLoader:0x140623647733240>

batch_loader.class
# batching...
# => Symbol

puts batch_loader
# foo
# => nil

At the same time, graphql-ruby gem implements a general adapter interface for different lazy execution mechanisms. This interface relies on an instance class method to detect whether an object is lazy or not. Unfortunately, since calling batch_loader.class tries to resolve the values, it is being resolved with N+1 queries by graphql-ruby gem itself.

batch_loader.class
# batching...
# => Symbol

I suggested a few other potentially more flexible solutions for graphql-ruby to detect lazy objects such as duck typing or using explicit arguments. But it looks like it won't be implemented. To fix the issue BatchLoader started wrapping BatchLoader objects with PORO (plain old ruby objects) by using graphql-ruby instrumentation.

This simple BatchLoader object wrapping stopped working since graphql-ruby version 1.8.7. This version introduced a breaking change and started detecting lazy objects before the instrumentation.

In order to fix the compatibility again, this PR introduces BatchLoader::GraphQL - a simple PORO with extra 20 lines of code to wrap BatchLoader without relying on graphql-ruby instrumentation. 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

Related to rmosolgo/graphql-ruby#1778. Fixes #22, #26, #30.

Repository owner deleted a comment from coveralls Jan 31, 2019
Repository owner deleted a comment from coveralls Jan 31, 2019
@exAspArk exAspArk merged commit 879cb72 into master Feb 1, 2019
@exAspArk exAspArk deleted the graphql branch February 1, 2019 18:02
@maletor
Copy link

maletor commented Feb 15, 2019

This created a regression where resolvers that do not return a BatchLoader::GraphQL fail to call BatchLoader::GraphQL calls that the resolver uses.

tigefa4u pushed a commit to tigefa4u/gitlabhq that referenced this pull request Aug 27, 2019
- Due to exAspArk/batch-loader#32,
we  changed BatchLoader.for into BatchLoader::GraphQL.for
- since our results are wrapped in a BatchLoader::GraphQL,
calling `sync` during authorization is required to get real object
- `graphql` now has it's own authorization system.  Our
`authorized?` method conflicted and required renaming
tigefa4u pushed a commit to tigefa4u/gitlabhq that referenced this pull request Aug 27, 2019
- Due to exAspArk/batch-loader#32,
we  changed BatchLoader.for into BatchLoader::GraphQL.for
- since our results are wrapped in a BatchLoader::GraphQL,
calling `sync` during authorization is required to get real object
- `graphql` now has it's own authorization system.  Our
`authorized?` method conflicted and required renaming
tigefa4u pushed a commit to tigefa4u/gitlabhq that referenced this pull request Aug 27, 2019
- Due to exAspArk/batch-loader#32,
we  changed BatchLoader.for into BatchLoader::GraphQL.for
- since our results are wrapped in a BatchLoader::GraphQL,
calling `sync` during authorization is required to get real object
- `graphql` now has it's own authorization system.  Our
`authorized?` method conflicted and required renaming
tigefa4u pushed a commit to tigefa4u/gitlabhq that referenced this pull request Aug 27, 2019
- Due to exAspArk/batch-loader#32,
we  changed BatchLoader.for into BatchLoader::GraphQL.for
- since our results are wrapped in a BatchLoader::GraphQL,
calling `sync` during authorization is required to get real object
- `graphql` now has it's own authorization system.  Our
`authorized?` method conflicted and required renaming
tigefa4u pushed a commit to tigefa4u/gitlabhq that referenced this pull request Aug 27, 2019
- Due to exAspArk/batch-loader#32,
we  changed BatchLoader.for into BatchLoader::GraphQL.for
- since our results are wrapped in a BatchLoader::GraphQL,
calling `sync` during authorization is required to get real object
- `graphql` now has it's own authorization system.  Our
`authorized?` method conflicted and required renaming
tigefa4u pushed a commit to tigefa4u/gitlabhq that referenced this pull request Aug 29, 2019
- Due to exAspArk/batch-loader#32,
we  changed BatchLoader.for into BatchLoader::GraphQL.for
- since our results are wrapped in a BatchLoader::GraphQL,
calling `sync` during authorization is required to get real object
- `graphql` now has it's own authorization system.  Our
`authorized?` method conflicted and required renaming
tigefa4u pushed a commit to tigefa4u/gitlabhq that referenced this pull request Aug 29, 2019
- Due to exAspArk/batch-loader#32,
we  changed BatchLoader.for into BatchLoader::GraphQL.for
- since our results are wrapped in a BatchLoader::GraphQL,
calling `sync` during authorization is required to get real object
- `graphql` now has it's own authorization system.  Our
`authorized?` method conflicted and required renaming
tigefa4u pushed a commit to tigefa4u/gitlabhq that referenced this pull request Aug 30, 2019
- Due to exAspArk/batch-loader#32,
we  changed BatchLoader.for into BatchLoader::GraphQL.for
- since our results are wrapped in a BatchLoader::GraphQL,
calling `sync` during authorization is required to get real object
- `graphql` now has it's own authorization system.  Our
`authorized?` method conflicted and required renaming
tigefa4u pushed a commit to tigefa4u/gitlabhq that referenced this pull request Aug 30, 2019
- Due to exAspArk/batch-loader#32,
we  changed BatchLoader.for into BatchLoader::GraphQL.for
- since our results are wrapped in a BatchLoader::GraphQL,
calling `sync` during authorization is required to get real object
- `graphql` now has it's own authorization system.  Our
`authorized?` method conflicted and required renaming
tigefa4u pushed a commit to tigefa4u/gitlabhq that referenced this pull request Aug 30, 2019
- Due to exAspArk/batch-loader#32,
we  changed BatchLoader.for into BatchLoader::GraphQL.for
- since our results are wrapped in a BatchLoader::GraphQL,
calling `sync` during authorization is required to get real object
- `graphql` now has it's own authorization system.  Our
`authorized?` method conflicted and required renaming
tigefa4u pushed a commit to tigefa4u/gitlabhq that referenced this pull request Sep 3, 2019
- Due to exAspArk/batch-loader#32,
we  changed BatchLoader.for into BatchLoader::GraphQL.for
- since our results are wrapped in a BatchLoader::GraphQL,
calling `sync` during authorization is required to get real object
- `graphql` now has it's own authorization system.  Our
`authorized?` method conflicted and required renaming
maxlazio pushed a commit to gitlabhq/gitlabhq that referenced this pull request Sep 4, 2019
- Due to exAspArk/batch-loader#32,
we  changed BatchLoader.for into BatchLoader::GraphQL.for
- since our results are wrapped in a BatchLoader::GraphQL,
calling `sync` during authorization is required to get real object
- `graphql` now has it's own authorization system.  Our
`authorized?` method conflicted and required renaming
tigefa4u pushed a commit to tigefa4u/gitlabhq that referenced this pull request Sep 9, 2019
- Due to exAspArk/batch-loader#32,
we  changed BatchLoader.for into BatchLoader::GraphQL.for
- since our results are wrapped in a BatchLoader::GraphQL,
calling `sync` during authorization is required to get real object
- `graphql` now has it's own authorization system.  Our
`authorized?` method conflicted and required renaming
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 this pull request may close these issues.

GraphQL v1.8.7 not compatible with Batch Loader
2 participants