Make the DataLoaderRegistry use a supplier pattern

[bbakerman]

See graphql-java/graphql-java#1902

benmccann commented 4 days ago

The DataLoaderRegistry should be request-scoped. That means that users are creating every DataLoader for every request even if none or only one or two are used.

Right now we would do something like:

dataLoaderRegistry.register("user", DataLoader.newDataLoader(userBatchLoader));

We could probably fix this by making DataLoaderReigstry a singleton and instead registering a function that would provide a DataLoader:

dataLoaderRegistry.register("user", () -> DataLoader.newDataLoader(userBatchLoaderProvider.get()));

[Inego]

Wouldn't this fix be a breaking change?

[Inego]

A non-breaking enhancement would be to add an optional field dataLoaderFactory to the DataLoaderRegistry.
This data loader factory, if set, would then be used in the getDataLoader function to act as the supplier of the required data loaders "on the fly" in case there is no such dataloader yet.
In the simplest, just a map of possible DL keys and their corresponding suppliers could serve as such factory.

[Inego]

An example: https://github.com/Inego/java-dataloader/commit/1c18495fcb78b89eb9ceebda585b7dbc76220677

[Inego]

In fact, there's a simpler/cleaner solution without an overhead for those who need this lazy behavior:

master...Inego:lazy-dl-registry

The usage would be as simple as:

// Prepare the singleton factory once...
LazyDataLoaderFactory factory = new LazyDataLoaderFactory();
        
factory.register("clients", () -> new ClientDataLoader());
factory.register("products", () -> new ProductDataLoader());

// ... and create a lazy registry for every request
LazyDataLoaderRegistry registry = factory.createRegistry();

[bbakerman]

I looked into this and its really kinda pointless.

We can make it lazy but as soon as you called dispatchAll the it needs to materialise the dataloaders and dispatch them

We would have to write a fancy MemoizingSupplierWithTracking that could decide if its every been asked for and then filter them out from the list. In short I am not sure that its worth it.

I would love to see more evidence now of the costs of creating a DL even if its not needed in a certain graphql request.