spring-graphql icon indicating copy to clipboard operation
spring-graphql copied to clipboard

Annotation based DataLoader registration

Open JamesPeters98 opened this issue 2 years ago • 3 comments

I'm currently starting a project after using Netflix's DGS framework that uses a @DgsDataLoader annotation to declare dataloaders as a class.

Is there a reason why an annotation based solution isn't available in Spring for GraphQL?

The @BatchMapping solution is useful for simple mappings, however it isn't useful when you have multiple mappings of the same type. E.g a collection Location and delivery Location.

Would it be possible register dataloaders in a similar way to how the DGS framework does it? As it currently doesn't feel very 'Spring'-like to register them 'programmatically'?

JamesPeters98 avatar Jul 06 '22 18:07 JamesPeters98

Indeed, @BatchMapping is only a shortcut but it is not required.

By default, BatchLoaderRegistry registers each DataLoader under the value class name, so for a DataLoader<Long, Location> controller method parameter, we can match by the generic type to find the DataLoader. You can, however, give explicit names in the BatchLoaderRegistry, and use those on controller method parameter names, e.g. DataLoader<Long, Location> collectionLocation or DataLoader<Long, Location> deliveryLocation. This is also in the docs.

I do understand the expectation for component detection, but it's not an automatic decision on our part, and not everything should be encouraged to be declared as a Spring bean, if it doesn't need to be.

For "data loaders", we felt there was quite a bit to know to begin with. All the BatchLoader contract variants, the DataLoader contract itself, as well as the actual registration of BatchLoader as a DataLoader under a specific name along with DataLoaderOptions. We also needed to add reactive support but didn't want to add more contracts to the mix, for what essentially comes down to the registration of one of two kinds of mapping functions (id's to List or Map) that are themselves typically simple adapters, and only need to be registered ones somewhere.

A registry is our way of presenting you with a centralized API for all this that guides you through a builder and exposes every available options from the underlying API, including all the DataLoaderOptions that are not otherwise feasible to expose as attributes on a component annotation.

rstoyanchev avatar Jul 07 '22 08:07 rstoyanchev

Thanks for the reply, I understand and agree that it shouldn't be annotation based by default.

I have been testing out the registry using these two generic functions:

    private <K, V> void registerMappedLoader(Class<K> c1, Class<V> c2, Function<Set<K>, Map<K, V>> supplier) {
        registry.forTypePair(c1, c2).registerMappedBatchLoader((ks, batchLoaderEnvironment) ->
            Mono.fromSupplier(() -> supplier.apply(ks))
            .subscribeOn(Schedulers.boundedElastic())
        );
    }

    private <K, V> void registerNamedLoader(String name, Function<Set<K>, Map<K,V>> supplier) {
        registry.<K, V>forName(name).registerMappedBatchLoader((ids, batchLoaderEnvironment) ->
            Mono.fromSupplier(() -> supplier.apply(ids))
            .subscribeOn(Schedulers.boundedElastic()));
    }

Now, is there any way to register a type pair DataLoader for a list, as currently I have to use the named DataLoader.

And if not, is there an option to enable checks to ensure the DataLoaders in @SchemaMapping function arguments exist at start up? Because currently it's very easy to mistype the named dataloader and it doesn't get caught until it's used in a query etc.

JamesPeters98 avatar Jul 12 '22 12:07 JamesPeters98

By "DataLoader for a list", do you mean a regular BatchLoader, as opposed to a MappedBatchLoader? From BatchLoaderRegistry#forName(name) you get a spec that lets you create either, so the API supports both equally. If you mean something else, then please clarify.

As for checks on DataLoader arguments in @SchemaMapping methods, the controller methods are introspected at a lower level while initializing GraphQL Java via GraphQlSource. By contrast, BatchLoaderRegistry is used in the layer above GraphQL Java since the DataLoaderRegistry is typically per request, and therefore, populated at runtime, just before invoking the engine.

That said, we could expose some method on BatchLoaderRegistry, or its base interface, to check whether a name has a corresponding registration, and pass that into AnnotatedControllerConfigurer which can then make such checks. This would be useful for registrations with an explicit name. Do you find that you have many such registrations, or is it fairly common to go with just a type pair?

rstoyanchev avatar Oct 12 '22 15:10 rstoyanchev

If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.

spring-projects-issues avatar Oct 19 '22 15:10 spring-projects-issues

Closing due to lack of requested feedback. If you would like us to look at this issue, please provide the requested information and we will re-open the issue.

spring-projects-issues avatar Oct 26 '22 15:10 spring-projects-issues