@BatchMapping support @Argument

[dugenkui03]

I think that both source and argument are necessary. It will great that @BatchingMapping support @Argument.

Schema like blow. 'like' means whether the visitor like this book or author, and Book.author bind DataLoader.

type Query{
      books(visitorId:Int, bookIds:[Int]):[Book]
}

type Book{
    id: Int
    like: Boolean
    authorId: Int
    author(visitorId:Int): Author
}

type Author{
    id: Int
    name: String
    like: Boolean
}

query like blow:

query ($visitorId: Int){
   books(visitorId:$visitorId, bookIds:[1,2,3]){
      authorId
      # need visitorId
       author(visitorId:$visitorId){
             id
       }
  }
}

[pinguinjkeke]

Any current workarounds for this?

[rstoyanchev]

A @BatchMapping method is a BatchLoader and that has access to BatchLoaderEnvironment which doesn't provide access to arguments. So there is no straight-forward way to do this.

More generally, my understanding is that a BatchLoader is just a contract for loading Objects given their id's and it could be invoked by GraphQL Java for different parts of the query which may have different arguments and different selection sets. So this is at a lower level where arguments as well as caching of values can't apply in a straight-forward way.

Note a @BatchMapping method has access to the same GraphQLContext instance as do @SchemaMapping methods, so you could pass context that way from a @SchemaMapping method and access it via BatchLoaderEnvironment#getContext().

There is also a more advanced option to use DataLoader directly (instead of the @BatchMapping method), and then you can pass more detailed "key context" for each item. Check the section on "Passing Context" in the GraphQL Java docs.

[rstoyanchev]

I'm closing this as there isn't anything specific we can add here, but feel free to comment further.

[dugenkui03]

@rstoyanchev I think user which use spring grpahql would not care about the underlying difference between @SchemaMapping and @BatchMapping: SchemaMappingDataFetcher just invoke method, and BatchMappingDataFetcher invoke method by DataLoad.

The only reason user use @BatchMapping is to avoid n+1 problem, and they still likely want the capacity which @SchemaMapping have.BatchLoader could get DataFetchingEnvironment by CompletableFuture<V> DataLoader#load(K key, Object keyContext) which could pass HandlerMethodArgumentResolver#resolveArgument, maybe more details need to handle.

Further more, I think it will be great that user could custom HandlerMethodArgumentResolver.

[rstoyanchev]

Re-opening to discuss further.

[rstoyanchev]

@dugenkui03 I closed the issue too soon perhaps, because I don't think the proposal is feasible. I see now that you had something concrete in mind for the implementation, so let me address it.

Your want to pass DataFetchingEnvironment through the per-key context, i.e. via DataLoader#load(K key, Object keyContext). That can work sometimes but not generally because DataLoader caches by key, and if there is anything that influences the result, other than the key, it becomes an issue.

Arguments are one of those things. We could make them available to BatchMapping methods, but it would be possible to use that to load a different entity, e.g. partially populated, etc. Once we pass arguments through, we might as well allow anything from DataFetchingEnvironment but there is a reason why that's not available in GraphQL Java.

@BatchMapping methods are meant to provide a simple shortcut to eliminate boilerplate for the most common case. If it doesn't do what you need, you don't have to use it and it's still quite straight forward:

@Controller
public class BookController {

    public BookController(BatchLoaderRegistry registry) {
        registry.forTypePair(Long.class, Author.class).registerMappedBatchLoader((authorIds, env) -> {
            // return Map<Long, Author>
        });
    }

    @SchemaMapping
    public CompletableFuture<Author> author(Book book, DataLoader<Long, Author> loader) {
        return loader.load(book.getAuthorId());
    }

}

[dugenkui03]

@rstoyanchev Thank for your response.

about cacheKey, and why DataLoader#load(K key, Object keyContext) always work

That can work sometimes but not generally because DataLoader caches by key, and if there is anything that influences the result, other than the key, it becomes an issue.

Developer can create cacheKey from both key and context(DataFetchingEnvironment), instead of only key. Details in DataLoaderHelper and CacheKey. The only thing we need to do is allow develop custom CacheKey.

In fact, I think DataLoader#load(K key, Object keyContext) will always work, because DataFetcher pass all the information(DataFetchingEnvironment) to DataLoader, and DataLoader can easily solve a variety of problems, such as cache thing. DataLoader is pretty flexible if we can custom DataLoaderOptions in spring-graphql.

not available in GraphQL Java

Sorry I could not understand this deeply, do you mean that java-dataloader could not get DataFetchingEnvironment?

There seems to be no limit.

The purpose of pr-324 and this talk

The point is that: we could make @BatchMapping as flexible as @SchemaMapping does, and the only difference between them should be batch-load and one-shot-invoke, and it is easy to achieve. And the pr-324 become how to use the DataLoader easily, instead of how to use it.

[rstoyanchev]

I think you're missing the main points.

One, @BatchMapping is a convenience that aims to hide the details of using a DataLoader. You point to customization of the caching key. That is a step in the opposite direction towards a deeper customization and for that, the @BatchMapping shortcut is not a good fit, just don't use it.

Two, I think the caching problem is deeper than you suspect. To work generally, the caching key would have to take into account the arguments used and the selection set. Imagine entities loaded for different parts of the response with different arguments and selection sets. There is a library called GOM that solves this problem in a general way but this is not something we are looking to do at this time.

As for this not being available in GraphQL Java, you can see this issue for example graphql-java/java-dataloader#26.

[dugenkui03]

@rstoyanchev Thanks very much. I misunderstand the design philosophy of @Schemamapping, and I find that graphql-dataloader haven't consider such a complex situation too.