Request-scoped DataLoader

[vojtechhabarta]

I am trying to use DataLoaders. If I understand it correctly:

• it is recommended to create new DataLoaderRegistry for each user request (to prevent mixing caches for different users)
• DataLoaderRegistry can be stored in custom context class which is instantiated in GraphQLServlet.createContext (in override or using GraphQLContextBuilder)
• in resolvers it is possible to get DataLoaderRegistry from this context
• DataLoaderDispatcherInstrumentation must be used so that dispatch methods are called

but here is the problem: DataLoaderDispatcherInstrumentation has DataLoaderRegistry and it is not request-scoped.

I haven't find a way to have request-scoped DataLoaderRegistry so I would like to improve the servlet to allow this use case. Or am I missing something?

Methods createContext and getInstrumentation need to somehow share new DataLoaderRegistry (per-request) instance. One solution would be to add methods getInstrumentation with GraphQLContext parameter, delegate these methods to their parameter-less versions and call this new method from newGraphQL method.

If you think this would be useful I can create PR (I already tried it). Or is there a better way?

[apottere]

Would it make sense to always add the DataLoaderDispatcherInstrumentation when the context has a DataLoaderRegistry? i.e. should it pass the context to the InstrumentationProvider or should it get the instrumentation provided by the InstrumentationProvider and then add the DataLoaderDispatcherInstrumentation to it automatically?

[vojtechhabarta]

Always adding DataLoaderDispatcherInstrumentation when the context has a DataLoaderRegistry would probably make sense, I think. But I don't know what exactly you mean. Something similar to what I tried in mentioned commit? In 60f1093 I added GraphQLContext parameter to InstrumentationProvider.getInstrumentation() method.

Or are you thinking about supporting DataLoaderDispatcherInstrumentation automatically and adding DataLoaderRegistry field to GraphQLContext?

[freedomljc]

@vojtechhabarta 's diff could help the GraphQLServlet to have per-request instrumentation.
Besides that, we need to add the dataloader into instrumentation.
To enable dataloader per request, GraphQLContext could be the best place to instantiate the dataloaders inside.

Ideas:

1. Add DataLoaderRegistry into the GraphQLContext.

public class GraphQLContext {
....
protected Optional dataloaderRegistry;
public GraphQLContext(Optional request, Optional response, Optional dataloderRegistry) {
this.request = request;
this.response = response;
this.dataloaderRegistry = dataloaderRegistry;
}
....
}

2. Create a new class ContextA to inherit the GraphQLContext:

public class ContextA extends GraphQLContext{
public ContextA() {
...
Dataloader<Object, Object> dataloaderA = new Dataloader<>();
DataLoaderRegistry registry = new DataLoaderRegistry();
this.registry.put("A", dataloaderA)
}

public CompleteFuture<A> loadAValue(Object key) {
	return this.registry.get("A").get(Key);
}

}

3. Create a new class InstrumentationProviderA to inherit the InstrumentationProvider (based on @vojtechhabarta 's diff), then be able to connect the dataloader with the object .
   public class InstrumentationProviderA implements InstrumentationProvider {
   public Instrumentation getInstrumentation(GraphQLContext context) {
   DataLoaderDispatcherInstrumentation dispatcherInstrumentation = new DataLoaderDispatcherInstrumentation(context.dataloaderRegistry);
   return dispatcherInstrumentation;
   }
   }

[vojtechhabarta]

I would like to solve this problem somehow.

I have 2 use cases:

• Objects that can be cached regardless the user accessing the data for some amount of time. For this use case current support for DataLoaders is sufficient, moreover this can also be solved using arbitrary cache.
• Object that can be cached only per user request. Here I cannot use normal cache. Since the structure is relatively complex and one object can appear on different levels of result it would be really beneficial to have per-request cache.

Any idea, solution, hack...?

[vincentDAO]

@vojtechhabarta, @freedomljc I LOVE your ideas to implement "instrumentation per request" .

Our project is using @deprecated BatchedExecutionStrategy we want to change to use DataLoader but stuck at configure DataLoader per request.

Have you guys tried to implement it?

[vojtechhabarta]

I haven't implemented it. I thought somebody who understands better than me how GraphQL in Java works should do it 😏

[ashu-walmart]

Will this PR help? #83.

[vincentDAO]

@ashu-walmart, hopefully your PR is reviewed and merged soon.

[kdlan]

I make a pull request graphql-java/graphql-java#1171 for this

with Supplier<DataLoaderRegistry>, DataLoaderDispatcherInstrumentation can get a request-scoped DataLoaderRegistry and FieldLevelTrackingApproach

[oliemansm]

@ashu-walmart et al. Anything still needed for this in this project? PR #83 has been merged. So like to know if there's still some functionality missing we need to implement. Also, if you're able to write a small readme explaining this feature for future reference would be much appreciated.

[ashu-walmart]

@oliemansm, This works for me now and am using the feature in my project. I will write a readme describing the feature. Thanks!