Skip to content

Looking for advice on DGS DataLoader migration #1233

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
cw-juyeonyu opened this issue Jun 4, 2025 · 4 comments
Open

Looking for advice on DGS DataLoader migration #1233

cw-juyeonyu opened this issue Jun 4, 2025 · 4 comments
Labels
for: team-attention An issue we need to discuss as a team to make progress status: waiting-for-triage An issue we've not yet triaged

Comments

@cw-juyeonyu
Copy link

Hi Spring GraphQL Team,

We're migrating from Netflix DGS to Spring GraphQL and have questions about DataLoader.

  • @SchemaMapping Argument Resolution Validation: When using a DataLoader within a @SchemaMapping method, how can we ensure mismatches between the DataLoader name (registered via BatchLoaderRegistry.forName()) and the method's parameter name intended for keys are caught at compile or startup time, rather than runtime?
  • Bean-based DataLoaders: Is there a recommended way to register DataLoaders as Spring beans, similar to DGS's @DgsDataLoader annotation, for easier management and auto-registration?

Any guidance on these points for a smoother migration would be appreciated.
Thanks!

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jun 4, 2025
@Crain-32
Copy link

Crain-32 commented Jun 8, 2025

Hey @cw-juyeonyu,

I'm not on the SpringQL Team, but I have answers to your questions, and I'd be interested in seeing what other ideas the Team brings.

@schemamapping Argument Resolution Validation: When using a DataLoader within a @schemamapping method, how can we ensure mismatches between the DataLoader name (registered via BatchLoaderRegistry.forName()) and the method's parameter name intended for keys are caught at compile or startup time, rather than runtime?

You can either test the method with a request, or you could do some verification using reflection. If you explore the reflection approach, you could also call it using @EventListener(ContextRefreshedEvent.class), and that would let you verify after all the Beans are setup and registered, but before the Application is taking traffic, which would give you the "Startup time" failure.

Bean-based DataLoaders: Is there a recommended way to register DataLoaders as Spring beans, similar to DGS's @DgsDataLoader annotation, for easier management and auto-registration?

The Spring Docs leverage @Controller, and based on that documentation it sounds like it has to be @Controller. You could do something similar to @DgsDataLoader and create a composed annotation with @Controller. This would also make the reflective verification mentioned earlier easier, because you can get all beans annotated with that composed annotation using the ApplicationContext::getBeansWithAnnotation.

@rstoyanchev
Copy link
Contributor

rstoyanchev commented Jun 9, 2025

Thanks for the feedback. I will provide some guidance and context.

Spring for GraphQL data fetchers (e.g. for @SchemaMapping methods) implement SelfDescribingDataFetcher, which exposes information about it that could be used, e.g. for the schema mapping inspection. The key is to get access to all data fetchers before decorators hide the SelfDescribingDataFetcher type. You could do it through a RuntimeWiringConfigurer that's ordered as late as possible, and compare it against what's in the BatchLoaderRegistry. Something like this:

@Bean
RuntimeWiringConfigurer runtimeWiringConfigurer(BatchLoaderRegistry batchLoaderRegistry) {

	DataLoaderRegistry dataLoaderRegistry = DataLoaderRegistry.newRegistry().build();
	batchLoaderRegistry.registerDataLoaders(dataLoaderRegistry, GraphQLContext.newContext().build());
	Map<String, DataLoader<?, ?>> dataLoaderMap = dataLoaderRegistry.getDataLoadersMap();

	return builder -> builder.build().getDataFetchers().values().stream()
			.flatMap(map -> map.values().stream())
			.forEach(fetcher -> {
				if (fetcher instanceof SelfDescribingDataFetcher<?> sddf) {
					Object source = sddf.getReturnType().getSource();
					if (source instanceof MethodParameter parameter) {
						for (Parameter param : parameter.getMethod().getParameters()) {
							if (param.getType().equals(DataLoader.class)) {
								// Check parameter against dataLoaderMap
							}
						}
					}
				}
			});
}

In short, it is not very straight forward, but should be possible. Generally, this could be a useful feature alongside the schema inspection that we could consider to make built-in or easier.

The BatchLoaderRegistry was created with the idea that batch loading is more of a simple function, that could often be a lambda callback. You could inject the registry into a controller and add the data loading function, or you could add many data loading functions to the registry from a central place. Or if you want to support those as beans, you could also create a bean that looks up other beans (by annotation or type) and registers them with the BatchLoaderRegistry.

@rstoyanchev rstoyanchev added the for: team-attention An issue we need to discuss as a team to make progress label Jun 9, 2025
@cw-juyeonyu
Copy link
Author

Thank you for your kind explanation.

@rstoyanchev
Copy link
Contributor

rstoyanchev commented Jun 10, 2025

No worries, and thanks for the feedback. We may choose to make DataLoader argument checks on startup a more fist-class feature, so I will leave this issue open just a little longer.

If you have further thoughts or comments based on your experience, please reach out.

@rstoyanchev rstoyanchev reopened this Jun 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for: team-attention An issue we need to discuss as a team to make progress status: waiting-for-triage An issue we've not yet triaged
Projects
None yet
Development

No branches or pull requests

4 participants