Skip to content
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

DataLoader registrations via BatchLoaderRegistry not used at runtime #1020

Closed
Vojtech-Sassmann opened this issue Jul 3, 2024 · 3 comments
Closed
Assignees
Labels
status: backported An issue that has been backported to maintenance branches type: regression A bug that is also a regression
Milestone

Comments

@Vojtech-Sassmann
Copy link

Vojtech-Sassmann commented Jul 3, 2024

Since version 1.3. we have detected a strange behavior.

We are using the BatchLoaderRegistry to register DataLoaders. We write some of them and some are generated using the graphql-maven-plugin.

Example of a DataLoader registration:

@Autowired // This is a constructor in a controller
public FooController(BatchLoaderRegistry registry) {
	registry.forTypePair(java.lang.Long.class, FooGQL.class)
			.registerMappedBatchLoader((keysSet, env) -> {
		....
	});

}

After upgrading to 1.3.X we have experienced behavior, that during runtime, there are no DataLoaders available. We were able to inspect this a little bit further and find a workaround. We have detected that on application startup, the BatchLoaderRegistry is invoked and there is a check, if some DataLoaders are configured. However, at this time, there are none of them, because the Controllers were not initialized.

Example exception:

java.lang.IllegalArgumentException: Cannot resolve DataLoader for parameter 'dataLoader' ... The DataLoaderRegistry contains: []

The workaround is to provide a custom Bean for the BatchLoaderRegistry and to initialize it with some data loaders.

    @Bean
    public BatchLoaderRegistry batchLoaderRegistry()
    {
        var batchLoaderRegistry = new DefaultBatchLoaderRegistry();
        batchLoaderRegistry
                .forTypePair(String.class, FooGQL.class)
                .withName("...")
                .registerMappedBatchLoader((guids, batchLoaderEnvironment) ->
                        Mono.fromCallable(() -> ...)
                );

        return batchLoaderRegistry;
    }

Is this expected behavior or is this a bug?

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jul 3, 2024
@rstoyanchev
Copy link
Contributor

You're probably referring to this check for #915.

I'll schedule this for a fix to ensure the check is performed later. That said #915 was fixed in 1.2.5 (and forward merged to 1.3.0). Can you confirm if you are perhaps upgrading from a version < 1.2.5. In other words, you should also see the issue with versions 1.2.5 - 1.2.7.

@rstoyanchev rstoyanchev self-assigned this Jul 3, 2024
@rstoyanchev rstoyanchev added type: regression A bug that is also a regression and removed status: waiting-for-triage An issue we've not yet triaged labels Jul 3, 2024
@rstoyanchev rstoyanchev added this to the 1.3.2 milestone Jul 3, 2024
@rstoyanchev rstoyanchev changed the title Change in registration of DataLoaders using BatchLoaderRegistry DataLoader registrations via BatchLoaderRegistry not used at runtime Jul 3, 2024
@Vojtech-Sassmann
Copy link
Author

You're probably referring to this check for #915.

I'll schedule this for a fix to ensure the check is performed later. That said #915 was fixed in 1.2.5 (and forward merged to 1.3.0). Can you confirm if you are perhaps upgrading from a version < 1.2.5. In other words, you should also see the issue with versions 1.2.5 - 1.2.7.

Yes, we were using version 1.2.4. Now I have also tested version 1.2.6 and I can confirm that even in this version I can see the same behavior as in the version 1.3.1.

@rstoyanchev rstoyanchev added the for: backport-to-1.2.x Marks an issue as a candidate for backport to 1.2.x label Jul 4, 2024
@github-actions github-actions bot added status: backported An issue that has been backported to maintenance branches and removed for: backport-to-1.2.x Marks an issue as a candidate for backport to 1.2.x labels Jul 4, 2024
@rstoyanchev
Copy link
Contributor

There is a fix now in 1.3.2-SNAPSHOT and 1.2.8-SNAPSHOT if you'd like to give it a try.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: backported An issue that has been backported to maintenance branches type: regression A bug that is also a regression
Projects
None yet
Development

No branches or pull requests

3 participants