spring-projects / spring-graphql

Spring Integration for GraphQL
https://spring.io/projects/spring-graphql
Apache License 2.0
1.52k stars 300 forks source link

DataLoader registrations via BatchLoaderRegistry not used at runtime #1020

Closed Vojtech-Sassmann closed 3 months ago

Vojtech-Sassmann commented 3 months ago

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?

rstoyanchev commented 3 months ago

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.

Vojtech-Sassmann commented 3 months ago

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 commented 3 months ago

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