DataLoader dispatches together keys from different requests about java-dataloader HOT 6 CLOSED

Wait... Why are you making DataLoaderRegistry a singleton if you want it per request? A singleton DataLoaderRegistry is only applicable to a very specific use-case and is not common at all.

What is normally done is having a DataLoaderRegistry created per request and stored into the global context for the execution, e.g.

DataLoaderRegistry dataLoaderRegistry = ...; // create per request

//Transform the pre-configured GraphQL instance or create a new one
GraphQL runtime = graphQL.transform(builder -> builder.instrumentation(
  new DataLoaderDispatcherInstrumentation(dataLoaderRegistry)));

//Make dataLoaderRegistry accessible to fetcher functions
ExecutionInput.newExecutionInput()
  .query(...)
  .context(dataLoaderRegistry)
  .build ();

This is very simple and requires no low-level concurrency control nor keeping track of executions. So I think there's nothing wrong with the current implementation.

from java-dataloader.

oh i see... thanks! i didn't realize that creating a GraphQL object was so lightweight. so it is indeed a problem with my setup.

By looking at graphql-spring-boots GraphQLWebAutoConfiguration.graphQLServlet(...), it looks like i have to declare my instrumentation and data loader registry beans with @RequestScope and then add a GraphQLContextBuilder that creates a context with the request-scoped registry. is that right?

from java-dataloader.

closing this, it's a non-issue, thanks a lot!

although i found it a bit unintuitive, so i'm leaving this here for other noobs like me, i had to do this:

    @Bean
    @RequestScope
    public DataLoaderRegistry dataLoaderRegistry() {
        ...
    }

    @Bean
    @RequestScope
    public Instrumentation instrumentation(DataLoaderRegistry dataLoaderRegistry) {
        return new DataLoaderDispatcherInstrumentation(dataLoaderRegistry);
    }

but because while ExecutionInput it's ok with any Object context, the GraphQLServlet.createContext(..) wants a GraphQLContext instance (see SimpleGraphQLServlets GraphQLContextBuilder field too) so I had to create a GraphQLContextBuilder implementation that returns a subclass of GraphQLContext instead of setting the registry directly. not a big deal, but i wonder if everyone is doing this or if people is using singletons without realizing the consequences? (or perhaps the is a more straightforward way that i'm not seeing)

to set the registry in the context i had to do this:

    @Bean
    public GraphQLContextBuilder graphQLContextBuilder(DataLoaderRegistry dataLoaderRegistry) {
        // note that dataLoaderRegistry is a request scoped proxy.
        return new GraphQLRequestContextBuilder(dataLoaderRegistry);
    }

and here are my context builder and context subclass:

public class GraphQLRequestContextBuilder implements GraphQLContextBuilder {
    private final DataLoaderRegistry dataLoaderRegistry;

    // ... constructor ...
    @Override
    public GraphQLContext build(Optional<HttpServletRequest> request, Optional<HttpServletResponse> response) {
        return new GraphQLRequestContext(request, response, dataLoaderRegistry);
    }
}

public class GraphQLRequestContext extends GraphQLContext {
    private final DataLoaderRegistry dataLoaderRegistry;
    
    public GraphQLRequestContext(Optional<HttpServletRequest> request, Optional<HttpServletResponse> response, DataLoaderRegistry dataLoaderRegistry) {
        super(request, response);
        this.dataLoaderRegistry = dataLoaderRegistry;
    }
   // ... getter ...
}

then my DataFetchers do:

DataFetchingEnvironment environment = ...;
GraphQLRequestContext context = environment.getContext();
DataLoaderRegistry registry = context.getDataLoaderRegistry();
DataLoader<K, V> dataLoader = registry.getDataLoader(name);
return dataLoader.load(key);

from java-dataloader.

from java-dataloader.

@edacostacambioupgrade I found the servlet overly convoluted, so I normally advise a simple Spring controller, as it's a lot more obvious.

I'm just curious, since your DataLoaderRegistry is already request scoped, you could directly inject it instead of keeping it in the context, right?

from java-dataloader.

@kaqqao i didn't try but i'm not sure i can inject them in my datafetchers because (i think) the request scoped proxies are somehow bound by spring to the current thread, i suspect spring will not find the right loader if the fetching happens a different thread.
(i guess something like this answer would be needed, or i would need to decorate the tasks of the executors to pass that around anywhere where an async task/thread is fired, but i didn't like either solution)

yeah, maybe having my own controller would be better, but by briefly looking at the servlet i see it has a lot of stuff (multipart handling, callbacks, etc) and i don't know enough to tell if i will need that, but i don't want to reimplement them in my controller if i eventually do.
i think i will end up subclassing the SimpleGraphQLServlet servlet, where i can create a new instrumentation and create the context without having to use request-scoped beans.

from java-dataloader.