Require loaders to be used within a GraphQL::Batch.batch block

[dylanahsmith]

@rmosolgo, @xuorig & @cjoudrey

Problem

As pointed out in #33 (comment) and #35, by storing state in thread local variables we run the risk of state leaking between requests or tests.

Solution

Change GraphQL::Batch::Executor.current so that it no longer automatically creates an executor so that the scope of batching is done explicitly using GraphQL::Batch.batch. E.g.

product = GraphQL::Batch.batch do
    RecordLoader.for(Product).load(product_id)
end

will assign an executor to GraphQL::Batch::Executor.current, call Promise.sync on the result of the block and clear the executor at the end of the block.

The reason why Promise.sync is needed on the result is because the promise callbacks might also do batch loads using GraphQL::Batch::Loader.for to get the loader from GraphQL::Batch::Executor.current.

[rmosolgo]

👍 In a world with no ExecutionStrategy, how do you think this could be hooked up to Schema#execute?

[dylanahsmith]

I was thinking it could be called outside of graphql-ruby around the call to Schema#execute. There is a before_query and after_query instrumentation in graphql-ruby, but an exception would prevent the after_query method from being called.

Do you have any suggestions for how you would like it to integrate with graphql-ruby?

[rmosolgo]

👍 for instrumentation, I think this ensure would do the trick: rmosolgo/graphql-ruby#412

Then you could have

GraphQL::Schema.define do 
  # ... 
  instrument(:query, GraphQL::Batch::Intstrumentation) 
end 

I'd be happy to update my open PR with that code if it looks ok to you!

[dylanahsmith]

That sounds good, except the name GraphQL::Batch::Instrumentation sounds like something for #27 rather than something to start and end the batch. What do you think about instrument(:query, GraphQL::Batch::Setup) to make its purpose clearer?

[rmosolgo]

👍 for Setup !

[dylanahsmith]

I'd be happy to update my open PR with that code if it looks ok to you!

👍