Skip to content

Support for BatchHandlerMethodArgumentResolver #324

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

Conversation

dugenkui03
Copy link
Contributor

The purpose of this PR

Replace hard code in BatchLoaderHandlerMethod with BatchHandlerMethodArgumentResolver, and support custom BatchHandlerMethodArgumentResolver.

1、Why spring-graphql should support BatchHandlerMethodArgumentResolver

User will define arguments on field, no matter whether the field is fetched by DataLoader or fetched directly( n+1 way). And user will always want to get additional information about field, such as Directives, FieldDefinition.

In conclusion, we'd better suport pass all the information of DataFetchingEnvironment to batch handler method. That said BatchLoaderHandlerMethod should support argumentResolve strategies which resolving argument values into various method parameters, and developers could custom their BatchLoaderHandlerMethod.

2、The way spring-graphql support BatchHandlerMethodArgumentResolver

Replace dataLoader.load(env.getSource()) with dataLoader.load(env.getSource(), env).

	static class BatchMappingDataFetcher implements DataFetcher<Object> {
                ......
		@Override
		public Object get(DataFetchingEnvironment env) {
			DataLoader<?, ?> dataLoader = env.getDataLoaderRegistry().getDataLoader(this.dataLoaderKey);
			if (dataLoader == null) {
				throw new IllegalStateException("No DataLoader for key '" + this.dataLoaderKey + "'");
			}
                         // pass DataFetchingEnvironment to DataLoader and other component
			return dataLoader.load(env.getSource(), env);
		}
	}

Brief change log

  • replace BatchLoaderHandlerMethod#resolveArgument with BatchHandlerMethodArgumentResolver, and support custom BatchLoaderHandlerMethod ;
  • add strategies: ArgumentBatchMethodArgumentResolverArgumentMapBatchMethodArgumentResolverArgumentsBatchMethodArgumentResolver and update document;
  • support for validate batchHandlerMethod input;
  • add unit tests for ArgumentBatchMethodArgumentResolver and the capability of custom BatchHandlerMethodArgumentResolver.

@larsduelfer
Copy link

larsduelfer commented Mar 25, 2022

@dugenkui03 When this PR is integrated, can I then use @BatchMapping having additional arguments beside the list of parent objects? This is what I am currently looking for to efficiently fetch child objects and apply filters on those as well.

@dugenkui03
Copy link
Contributor Author

dugenkui03 commented Mar 30, 2022

@larsduelfer I think this pr may not merged into main branch. there is an issue about this pr #232

issue-232: @BatchMapping methods are meant to provide a simple shortcut to eliminate boilerplate for the most common case.

You can use dataLoader with arguments in this way:

@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());
    }
}

apply filters on those

Even in the way of @BatchMapping, you still could not filter list in batch method, because you must assurence that the number of return list/map is equal to the number of parent-source-list.

@dugenkui03 dugenkui03 closed this Apr 3, 2022
@rstoyanchev rstoyanchev removed the status: waiting-for-triage An issue we've not yet triaged label Nov 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants