Add context to dataloader creation

[josephlbarnett]

To allow setting the GraphQLContext as the
batch context in a dataloader, pass it through
the KotlinDataLoaderRegistryFactory.generate()
call.

( discussed in https://kotlinlang.slack.com/archives/CQLNT7B29/p1659112178486379?thread_ts=1659049940.071919&cid=CQLNT7B29 )

[josephlbarnett]

left this method in the interface for backward compatibility reasons, but if we don't mind breaking that could remove it and only have the context-taking getDataLoader method

[josephlbarnett]

example of how code could use this to set the batch loader context to the GraphQLContext

[samuelAndalon]

Hey @josephlbarnett first of all thank you for the PR, contributions are always welcomed!

i would like to mention a couple of thoughts with this approach:

1. As mentioned in the slack conversation, we kind of solved this problem in a different way using the BatchLoaderEnvironment keys context as it feels more natural and adds access not only to the GraphQLContext but also the whole DataFetchingEnvironment, at the end the GraphQLContext exists only once in memory and we would be just working with references
2. With this approach we are kind of adding an implicit dependency between the DataLoaders and the GraphQLContext as the only option for a context in a BatchLoaderEnvironment will only be GraphQLContext, in any case i would check if we can work with generics instead of an explicit type

having pointed all these thoughts i just think that passing the GraphQLContext by key would just work fine and you can just leverage on the extension fuctions that we added to facilitate the access to the context

DataLoaderFactory.newDataLoader { requests, environment: BatchLoaderEnvironment ->
  val context = environment.getGraphQLContext()
  // do something with requests and context
 }

[samuelAndalon]

FYI these extension functions were released on version 6.1.0

[josephlbarnett]

Hey @samuelAndalon , thanks for your reply here. My concern with the keys approach is less about performance/memory, and more about semantics of the batch contexts. Looking at the graphql-java docs on this, my read is that the keyContexts really can/should be able to be different for each object loaded by the batch loader. While I don't really have a use case for that, an approach that forces it to be the same for every loaded object when there is already an existing "overall context" object is (to me at least) a surprising/confusing one when reading the code later, or writing new resolvers that need to remember to call the correctly overloaded .load() method.

I have updated the PR to be a little less tied to GraphQLContext, and instead depend on a DataLoaderOptions per request, which allows control of the options beyond just the context object, which I think is a good change. Generating that options object (in a new KotlinDataLoaderOptionsFactory class) is still tied to GraphQLContext though so not sure it addresses your generic concern? Could potentially also pass the GraphQLRequest through to generate the DataLoaderOptions, but not sure that's necessary?

[samuelAndalon]

@josephlbarnett we had an internal discussion about this and we don't think its a good idea,
as mentioned we solved this problem by passing the DataFetchingEnvironment per each load call.

about your concern

an approach that forces it to be the same for every loaded object

this is partially incorrect, as DataFetchingEnvironment is unique per data fetcher, which happens to have a reference to the GraphQLContext.

Try the approach we recommend using the extension fuctions and passing the dataFetchingEnvironment to each load invokation

[josephlbarnett]

ok thanks for the correction there, and thanks for having the discussion. Your recommended approach definitely does work.

We'll probably still do something wrapping KotlinDataLoaderRegistryFactory to expose the graphql context to the DataLoaderOptions so that we only have to think about it when constructing the dataloaders and not in each resolver that calls load(), but respect your team's decision on this.