Repository navigation
Make DataLoader a 1st class entity. #1254
Description
Today the data loader support is opt in via Instrumentation.
So you need to do this to opt in
DataLoaderRegistry registry = new DataLoaderRegistry();
registry.register("character", characterDataLoader);
DataLoaderDispatcherInstrumentation dispatcherInstrumentation
= new DataLoaderDispatcherInstrumentation(registry);
GraphQL graphQL = GraphQL.newGraphQL(buildSchema())
.instrumentation(dispatcherInstrumentation)
.build();
However for batching data, DataLoader is the preferred option. So it should not need you to Opt In but rather just work.
Another design challenge is that DataLoaders typically have a per request lifecycle but Instrumentation has an app level lifecycle.
To fix this, I propose that DataLoaderDispatcherInstrumentation no longer needs to be added and DataLoaderRegistry gets moved to the ExecutionInput and hence per request scope.
We would add getDataLoader("loaderName) to DataFetchingEnvironment to make calling it even easier
So it would become
GraphQL graphQL = GraphQL.newGraphQL(buildSchema())
.build();
// later per request
DataLoaderRegistry registry = new DataLoaderRegistry();
registry.register("character", characterDataLoader);
ExecutionInput executionInput = ExecutionInput.newExecutionInput().query("queryStr").dataLoaderRegistry(registry).build();
ExecutionResult er = graphql.execute(executionInput);
and in a Data Fetcher
DataFetcher df = dataFetchingEnvironment -> {
return dataFetchingEnvironment.getDataLoader("character").get(key)
};
Activity
tinnou commentedon Oct 2, 2018
So if I understand correctly, the
DataLoaderRegistrywould now be supplied per request. What aboutDataLoaderandBatchLoader? It seems that they are still reused across requests, right?andimarek commentedon Oct 2, 2018
@tinnou
BatchLoadercan be reused across requests, but eachDataLoaderRegistryshould be come with newDataLoadersinstances.tinnou commentedon Oct 2, 2018
ok sounds good, it's definitely not the way we have been using it, so it's good that this approach makes it harder to do the wrong thing.
bbakerman commentedon Oct 3, 2018
@tinnou - a DataLoader is also a cache by default SO IF you really want to share data between requests, you could use a new cache implementation (its pluggable) such that it put the data in a shared place (memcache / redis etc..) and hence the key -> value could be shared across requests if need be.
But if we create DataLoaders per request, then a dispatch on them ONLY affects this request and not other ones. Thats what we want to achieve and I think this is better API to show that.
Before out API encouraged the sharing of DataLoaders instances and thats not a great thing in practice.