From 1bdcb8afe8a50eb034309fbc0174f6b9a1ed89c8 Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Tue, 14 Sep 2021 09:11:10 +0100 Subject: [PATCH] Add DataLoaderMethodArgumentResolver This commit adds support for DataLoader arguments on annotated handler methods, where the key for the lookup is the class name of the value type or the parameter name. See gh-63 --- .../boot/GraphQlWebFluxAutoConfiguration.java | 6 +- .../boot/GraphQlWebMvcAutoConfiguration.java | 6 +- .../AnnotatedDataFetcherConfigurer.java | 3 + .../DataLoaderMethodArgumentResolver.java | 117 ++++++++++++++ ...> AnnotatedDataFetcherDetectionTests.java} | 2 +- .../AnnotatedDataFetcherInvocationTests.java | 105 ++++++++++--- .../DataLoaderArgumentResolverTests.java | 144 ++++++++++++++++++ 7 files changed, 352 insertions(+), 31 deletions(-) create mode 100644 spring-graphql/src/main/java/org/springframework/graphql/data/method/annotation/support/DataLoaderMethodArgumentResolver.java rename spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/{AnnotatedDataFetcherConfigurerTests.java => AnnotatedDataFetcherDetectionTests.java} (99%) create mode 100644 spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/DataLoaderArgumentResolverTests.java diff --git a/graphql-spring-boot-starter/src/main/java/org/springframework/graphql/boot/GraphQlWebFluxAutoConfiguration.java b/graphql-spring-boot-starter/src/main/java/org/springframework/graphql/boot/GraphQlWebFluxAutoConfiguration.java index a74123ec..5bcc9629 100644 --- a/graphql-spring-boot-starter/src/main/java/org/springframework/graphql/boot/GraphQlWebFluxAutoConfiguration.java +++ b/graphql-spring-boot-starter/src/main/java/org/springframework/graphql/boot/GraphQlWebFluxAutoConfiguration.java @@ -76,9 +76,9 @@ public class GraphQlWebFluxAutoConfiguration { @Bean public AnnotatedDataFetcherConfigurer annotatedDataFetcherConfigurer(ServerCodecConfigurer configurer) { - AnnotatedDataFetcherConfigurer registrar = new AnnotatedDataFetcherConfigurer(); - registrar.setServerCodecConfigurer(configurer); - return registrar; + AnnotatedDataFetcherConfigurer dataFetcherConfigurer = new AnnotatedDataFetcherConfigurer(); + dataFetcherConfigurer.setServerCodecConfigurer(configurer); + return dataFetcherConfigurer; } @Bean diff --git a/graphql-spring-boot-starter/src/main/java/org/springframework/graphql/boot/GraphQlWebMvcAutoConfiguration.java b/graphql-spring-boot-starter/src/main/java/org/springframework/graphql/boot/GraphQlWebMvcAutoConfiguration.java index b3e53bad..739fbd18 100644 --- a/graphql-spring-boot-starter/src/main/java/org/springframework/graphql/boot/GraphQlWebMvcAutoConfiguration.java +++ b/graphql-spring-boot-starter/src/main/java/org/springframework/graphql/boot/GraphQlWebMvcAutoConfiguration.java @@ -84,9 +84,9 @@ public class GraphQlWebMvcAutoConfiguration { @Bean public AnnotatedDataFetcherConfigurer annotatedDataFetcherConfigurer(HttpMessageConverters converters) { - AnnotatedDataFetcherConfigurer registrar = new AnnotatedDataFetcherConfigurer(); - registrar.setJsonMessageConverter(getJsonConverter(converters)); - return registrar; + AnnotatedDataFetcherConfigurer dataFetcherConfigurer = new AnnotatedDataFetcherConfigurer(); + dataFetcherConfigurer.setJsonMessageConverter(getJsonConverter(converters)); + return dataFetcherConfigurer; } @SuppressWarnings("unchecked") diff --git a/spring-graphql/src/main/java/org/springframework/graphql/data/method/annotation/support/AnnotatedDataFetcherConfigurer.java b/spring-graphql/src/main/java/org/springframework/graphql/data/method/annotation/support/AnnotatedDataFetcherConfigurer.java index 69d52e0d..43a8cce9 100644 --- a/spring-graphql/src/main/java/org/springframework/graphql/data/method/annotation/support/AnnotatedDataFetcherConfigurer.java +++ b/spring-graphql/src/main/java/org/springframework/graphql/data/method/annotation/support/AnnotatedDataFetcherConfigurer.java @@ -161,6 +161,9 @@ public class AnnotatedDataFetcherConfigurer this.argumentResolvers.addResolver(initInputArgumentMethodArgumentResolver()); this.argumentResolvers.addResolver(new ArgumentMapMethodArgumentResolver()); this.argumentResolvers.addResolver(new DataFetchingEnvironmentMethodArgumentResolver()); + this.argumentResolvers.addResolver(new DataLoaderMethodArgumentResolver()); + + // This works as a fallback, after all other resolvers this.argumentResolvers.addResolver(new SourceMethodArgumentResolver()); } diff --git a/spring-graphql/src/main/java/org/springframework/graphql/data/method/annotation/support/DataLoaderMethodArgumentResolver.java b/spring-graphql/src/main/java/org/springframework/graphql/data/method/annotation/support/DataLoaderMethodArgumentResolver.java new file mode 100644 index 00000000..e08fa0e0 --- /dev/null +++ b/spring-graphql/src/main/java/org/springframework/graphql/data/method/annotation/support/DataLoaderMethodArgumentResolver.java @@ -0,0 +1,117 @@ +/* + * Copyright 2002-2021 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.springframework.graphql.data.method.annotation.support; + +import java.lang.reflect.ParameterizedType; +import java.lang.reflect.Type; + +import graphql.schema.DataFetchingEnvironment; +import org.dataloader.DataLoader; + +import org.springframework.core.MethodParameter; +import org.springframework.core.ResolvableType; +import org.springframework.graphql.data.method.HandlerMethodArgumentResolver; +import org.springframework.lang.Nullable; +import org.springframework.util.Assert; + +/** + * Resolver that retrieves arguments of type {@link DataLoader} from the + * {@link DataFetchingEnvironment}. + * + *

The {@code DataLoader} is looked up by deriving the key using one of the following: + *

    + *
  1. The full name of the value type from the DataLoader generic types.
    2. + *
  2. The method parameter name.
    3. + *
+ * + * @author Rossen Stoyanchev + * @since 1.0.0 + */ +public class DataLoaderMethodArgumentResolver implements HandlerMethodArgumentResolver { + + @Override + public boolean supportsParameter(MethodParameter parameter) { + return parameter.getParameterType().equals(DataLoader.class); + } + + @Override + public Object resolveArgument(MethodParameter parameter, DataFetchingEnvironment environment) { + DataLoader dataLoader = null; + + Class valueType = getValueType(parameter); + if (valueType != null) { + dataLoader = environment.getDataLoader(valueType.getName()); + } + + String parameterName = null; + if (dataLoader == null) { + parameterName = parameter.getParameterName(); + if (parameterName != null) { + dataLoader = environment.getDataLoader(parameterName); + } + } + + if (dataLoader == null) { + String message = getErrorMessage(parameter, environment, valueType, parameterName); + throw new IllegalArgumentException(message); + } + + return dataLoader; + } + + @Nullable + private Class getValueType(MethodParameter param) { + Assert.isAssignable(DataLoader.class, param.getParameterType()); + Type genericType = param.getGenericParameterType(); + if (genericType instanceof ParameterizedType) { + ParameterizedType parameterizedType = (ParameterizedType) genericType; + if (parameterizedType.getActualTypeArguments().length == 2) { + Type valueType = parameterizedType.getActualTypeArguments()[1]; + return (valueType instanceof Class ? + (Class) valueType : ResolvableType.forType(valueType).resolve()); + } + } + return null; + } + + private String getErrorMessage( + MethodParameter parameter, DataFetchingEnvironment environment, + @Nullable Class valueType, @Nullable String parameterName) { + + String message = "Cannot resolve DataLoader for parameter" + + (parameterName != null ? " '" + parameterName + "'" : "[" + parameter.getParameterIndex() + "]" ) + + " in method " + parameter.getMethod().toGenericString() + ". "; + + if (valueType == null) { + message += "If the batch loader was registered without a name, " + + "then declaring the DataLoader argument with generic types should help " + + "to look up the DataLoader based on the value type name."; + } + else if (parameterName == null) { + message += "If the batch loader was registered with a name, " + + "then compiling with \"-parameters\" should help " + + "to look up the DataLoader based on the parameter name."; + } + else { + message += "Neither the name of the declared value type '" + valueType + "' " + + "nor the method parameter name '" + parameterName + "' match to any DataLoader. " + + "The DataLoaderRegistry contains: " + environment.getDataLoaderRegistry().getKeys(); + } + + return message; + } + +} diff --git a/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/AnnotatedDataFetcherConfigurerTests.java b/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/AnnotatedDataFetcherDetectionTests.java similarity index 99% rename from spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/AnnotatedDataFetcherConfigurerTests.java rename to spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/AnnotatedDataFetcherDetectionTests.java index 54a6e94c..7a7882ff 100644 --- a/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/AnnotatedDataFetcherConfigurerTests.java +++ b/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/AnnotatedDataFetcherDetectionTests.java @@ -42,7 +42,7 @@ import static org.assertj.core.api.Assertions.assertThat; * * @author Rossen Stoyanchev */ -public class AnnotatedDataFetcherConfigurerTests { +public class AnnotatedDataFetcherDetectionTests { @Test void registerWithDefaultCoordinates() { diff --git a/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/AnnotatedDataFetcherInvocationTests.java b/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/AnnotatedDataFetcherInvocationTests.java index ce42bbf1..b58c8114 100644 --- a/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/AnnotatedDataFetcherInvocationTests.java +++ b/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/AnnotatedDataFetcherInvocationTests.java @@ -17,28 +17,36 @@ package org.springframework.graphql.data.method.annotation.support; import java.util.List; import java.util.Map; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.atomic.AtomicReference; -import graphql.ExecutionInput; import graphql.ExecutionResult; -import graphql.GraphQL; import graphql.GraphQLContext; import graphql.schema.DataFetchingEnvironment; +import org.dataloader.DataLoader; import org.junit.jupiter.api.Test; import org.reactivestreams.Publisher; import reactor.core.publisher.Flux; import reactor.test.StepVerifier; import org.springframework.context.annotation.AnnotationConfigApplicationContext; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; import org.springframework.core.io.ClassPathResource; import org.springframework.graphql.Author; import org.springframework.graphql.Book; import org.springframework.graphql.BookCriteria; import org.springframework.graphql.BookSource; +import org.springframework.graphql.GraphQlService; +import org.springframework.graphql.RequestInput; import org.springframework.graphql.data.method.annotation.Argument; import org.springframework.graphql.data.method.annotation.MutationMapping; import org.springframework.graphql.data.method.annotation.QueryMapping; import org.springframework.graphql.data.method.annotation.SchemaMapping; import org.springframework.graphql.data.method.annotation.SubscriptionMapping; +import org.springframework.graphql.execution.BatchLoaderRegistry; +import org.springframework.graphql.execution.DefaultBatchLoaderRegistry; +import org.springframework.graphql.execution.ExecutionGraphQlService; import org.springframework.graphql.execution.GraphQlSource; import org.springframework.http.codec.ServerCodecConfigurer; import org.springframework.stereotype.Controller; @@ -64,7 +72,9 @@ public class AnnotatedDataFetcherInvocationTests { " }" + "}"; - ExecutionResult result = initGraphQl(BookController.class).execute(query); + ExecutionResult result = initGraphQlService(BookController.class) + .execute(new RequestInput(query, null, null)) + .block(); assertThat(result.getErrors()).isEmpty(); Map data = result.getData(); @@ -88,7 +98,9 @@ public class AnnotatedDataFetcherInvocationTests { " }" + "}"; - ExecutionResult result = initGraphQl(BookController.class).execute(query); + ExecutionResult result = initGraphQlService(BookController.class) + .execute(new RequestInput(query, null, null)) + .block(); assertThat(result.getErrors()).isEmpty(); Map data = result.getData(); @@ -110,8 +122,16 @@ public class AnnotatedDataFetcherInvocationTests { " }" + "}"; - ExecutionInput input = ExecutionInput.newExecutionInput().query(query).build(); - ExecutionResult result = initGraphQl(BookController.class).execute(input); + AtomicReference contextRef = new AtomicReference<>(); + RequestInput requestInput = new RequestInput(query, null, null); + requestInput.configureExecutionInput((executionInput, builder) -> { + contextRef.set(executionInput.getGraphQLContext()); + return executionInput; + }); + + ExecutionResult result = initGraphQlService(BookController.class) + .execute(requestInput) + .block(); assertThat(result.getErrors()).isEmpty(); Map data = result.getData(); @@ -122,7 +142,7 @@ public class AnnotatedDataFetcherInvocationTests { assertThat(author.get("firstName")).isEqualTo("George"); assertThat(author.get("lastName")).isEqualTo("Orwell"); - assertThat(input.getGraphQLContext().get("key")).isEqualTo("value"); + assertThat(contextRef.get().get("key")).isEqualTo("value"); } @Test @@ -135,7 +155,9 @@ public class AnnotatedDataFetcherInvocationTests { " }" + "}"; - ExecutionResult result = initGraphQl(BookController.class).execute(operation); + ExecutionResult result = initGraphQlService(BookController.class) + .execute(new RequestInput(operation, null, null)) + .block(); assertThat(result.getErrors()).isEmpty(); Map data = result.getData(); @@ -156,7 +178,9 @@ public class AnnotatedDataFetcherInvocationTests { " }" + "}"; - ExecutionResult result = initGraphQl(BookController.class).execute(operation); + ExecutionResult result = initGraphQlService(BookController.class) + .execute(new RequestInput(operation, null, null)) + .block(); assertThat(result.getErrors()).isEmpty(); Publisher publisher = result.getData(); @@ -180,22 +204,12 @@ public class AnnotatedDataFetcherInvocationTests { } - private GraphQL initGraphQl(Class beanClass) { + private ExecutionGraphQlService initGraphQlService(Class beanClass) { AnnotationConfigApplicationContext applicationContext = new AnnotationConfigApplicationContext(); - applicationContext.registerBean(beanClass); + applicationContext.register(TestConfig.class); applicationContext.refresh(); - AnnotatedDataFetcherConfigurer configurer = new AnnotatedDataFetcherConfigurer(); - configurer.setApplicationContext(applicationContext); - configurer.setServerCodecConfigurer(ServerCodecConfigurer.create()); - configurer.afterPropertiesSet(); - - GraphQlSource graphQlSource = GraphQlSource.builder() - .schemaResources(new ClassPathResource("books/schema.graphqls")) - .configureRuntimeWiring(configurer::configure) - .build(); - - return graphQlSource.graphQl(); + return applicationContext.getBean(ExecutionGraphQlService.class); } @SuppressWarnings("unchecked") @@ -204,9 +218,52 @@ public class AnnotatedDataFetcherInvocationTests { } + @Configuration + static class TestConfig { + + @Bean + public BookController bookController() { + return new BookController(batchLoaderRegistry()); + } + + @Bean + public GraphQlService graphQlService(GraphQlSource graphQlSource) { + ExecutionGraphQlService service = new ExecutionGraphQlService(graphQlSource); + service.addDataLoaderRegistrar(batchLoaderRegistry()); + return service; + } + + @Bean + public GraphQlSource graphQlSource() { + return GraphQlSource.builder() + .schemaResources(new ClassPathResource("books/schema.graphqls")) + .configureRuntimeWiring(annotatedDataFetcherConfigurer()) + .build(); + } + + @Bean + public AnnotatedDataFetcherConfigurer annotatedDataFetcherConfigurer() { + AnnotatedDataFetcherConfigurer registrar = new AnnotatedDataFetcherConfigurer(); + registrar.setServerCodecConfigurer(ServerCodecConfigurer.create()); + return registrar; + } + + @Bean + public DefaultBatchLoaderRegistry batchLoaderRegistry() { + return new DefaultBatchLoaderRegistry(); + } + + } + + @Controller private static class BookController { + public BookController(BatchLoaderRegistry batchLoaderRegistry) { + batchLoaderRegistry.forTypePair(Long.class, Author.class) + .registerBatchLoader((ids, env) -> Flux.fromIterable(ids).map(BookSource::getAuthor)); + } + @QueryMapping public Book bookById(@Argument Long id) { return BookSource.getBookWithoutAuthor(id); @@ -218,8 +275,8 @@ public class AnnotatedDataFetcherInvocationTests { } @SchemaMapping - public Author author(Book book) { - return BookSource.getBook(book.getId()).getAuthor(); + public CompletableFuture author(Book book, DataLoader dataLoader) { + return dataLoader.load(book.getAuthorId()); } @QueryMapping diff --git a/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/DataLoaderArgumentResolverTests.java b/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/DataLoaderArgumentResolverTests.java new file mode 100644 index 00000000..3d3be35a --- /dev/null +++ b/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/DataLoaderArgumentResolverTests.java @@ -0,0 +1,144 @@ +/* + * Copyright 2002-2021 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.springframework.graphql.data.method.annotation.support; + +import java.lang.reflect.Method; +import java.util.function.Consumer; + +import graphql.schema.DataFetchingEnvironment; +import graphql.schema.DataFetchingEnvironmentImpl; +import org.dataloader.DataLoader; +import org.dataloader.DataLoaderRegistry; +import org.junit.jupiter.api.Test; +import reactor.core.publisher.Flux; + +import org.springframework.core.DefaultParameterNameDiscoverer; +import org.springframework.core.MethodParameter; +import org.springframework.graphql.Author; +import org.springframework.graphql.Book; +import org.springframework.graphql.execution.BatchLoaderRegistry; +import org.springframework.graphql.execution.DefaultBatchLoaderRegistry; +import org.springframework.util.ClassUtils; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +/** + * Unit tests for {@link DataLoaderMethodArgumentResolver}. + * + * @author Rossen Stoyanchev + */ +public class DataLoaderArgumentResolverTests { + + private static final Method method = ClassUtils.getMethod( + DataLoaderArgumentResolverTests.class, "handle", (Class[]) null); + + private final DataLoaderMethodArgumentResolver resolver = new DataLoaderMethodArgumentResolver(); + + + @Test + void supportsParameter() { + assertThat(this.resolver.supportsParameter(initParameter(0))).isTrue(); + assertThat(this.resolver.supportsParameter(initParameter(3))).isFalse(); + } + + @Test + void resolveArgument() { + DataFetchingEnvironment environment = initEnvironment(registry -> + registry.forTypePair(Long.class, Author.class).registerBatchLoader((ids, env) -> Flux.empty())); + + Object argument = this.resolver.resolveArgument(initParameter(0), environment); + assertThat(argument).isNotNull(); + } + + @Test + void resolveArgumentViaParameterName() { + DataFetchingEnvironment environment = initEnvironment(registry -> + registry.forTypePair(Long.class, Author.class) + .withName("namedDataLoader") + .registerBatchLoader((ids, env) -> Flux.empty())); + + MethodParameter parameter = initParameter(1); + parameter.initParameterNameDiscovery(new DefaultParameterNameDiscoverer()); + + Object argument = this.resolver.resolveArgument(parameter, environment); + assertThat(argument).isNotNull(); + } + + @Test + void resolveArgumentFailureWithoutGenericType() { + DataFetchingEnvironment environment = initEnvironment(registry -> + registry.forTypePair(Long.class, Author.class).registerBatchLoader((ids, env) -> Flux.empty())); + + assertThatThrownBy(() -> this.resolver.resolveArgument(initParameter(2), environment)) + .hasMessageContaining("declaring the DataLoader argument with generic types should help"); + } + + @Test + void resolveArgumentFailureWithoutParameterName() { + DataFetchingEnvironment environment = initEnvironment(registry -> + registry.forTypePair(Long.class, Author.class) + .withName("namedDataLoader") + .registerBatchLoader((ids, env) -> Flux.empty())); + + MethodParameter parameter = initParameter(1); + // Skip ParameterNameDiscovery + + assertThatThrownBy(() -> this.resolver.resolveArgument(parameter, environment)) + .hasMessageContaining("compiling with \"-parameters\" should help"); + } + + @Test + void resolveArgumentFailureNoMatch() { + DataFetchingEnvironment environment = initEnvironment(registry -> + registry.forTypePair(Long.class, Book.class) + .withName("bookDataLoader") + .registerBatchLoader((ids, env) -> Flux.empty())); + + MethodParameter parameter = initParameter(0); + parameter.initParameterNameDiscovery(new DefaultParameterNameDiscoverer()); + + assertThatThrownBy(() -> this.resolver.resolveArgument(parameter, environment)) + .hasMessageContaining( + "Neither the name of the declared value type 'class org.springframework.graphql.Author' " + + "nor the method parameter name 'authorDataLoader' match to any DataLoader. " + + "The DataLoaderRegistry contains: [bookDataLoader]"); + } + + private DataFetchingEnvironment initEnvironment(Consumer registryConsumer) { + DefaultBatchLoaderRegistry batchLoaderRegistry = new DefaultBatchLoaderRegistry(); + registryConsumer.accept(batchLoaderRegistry); + + DataLoaderRegistry registry = DataLoaderRegistry.newRegistry().build(); + batchLoaderRegistry.registerDataLoaders(registry); + + return DataFetchingEnvironmentImpl.newDataFetchingEnvironment().dataLoaderRegistry(registry).build(); + } + + private MethodParameter initParameter(int index) { + return new MethodParameter(method, index); + } + + + @SuppressWarnings({"unused", "rawtypes"}) + public void handle( + DataLoader authorDataLoader, + DataLoader namedDataLoader, + DataLoader rawDataLoader, + DataFetchingEnvironment environment) { + } + +}