From 65be8ed35ff4524316c05395dd69916bff874128 Mon Sep 17 00:00:00 2001 From: rstoyanchev Date: Fri, 9 Sep 2022 07:16:47 +0100 Subject: [PATCH 1/3] Do not skip DataFetcherFactories Closes gh-440 --- .../ContextDataFetcherDecorator.java | 14 +++- .../ContextDataFetcherDecoratorTests.java | 68 ++++++++++++++++++- 2 files changed, 76 insertions(+), 6 deletions(-) diff --git a/spring-graphql/src/main/java/org/springframework/graphql/execution/ContextDataFetcherDecorator.java b/spring-graphql/src/main/java/org/springframework/graphql/execution/ContextDataFetcherDecorator.java index 1f2d3d9b..06696d99 100644 --- a/spring-graphql/src/main/java/org/springframework/graphql/execution/ContextDataFetcherDecorator.java +++ b/spring-graphql/src/main/java/org/springframework/graphql/execution/ContextDataFetcherDecorator.java @@ -109,14 +109,14 @@ final class ContextDataFetcherDecorator implements DataFetcher { return new GraphQLTypeVisitorStub() { @Override - public TraversalControl visitGraphQLFieldDefinition(GraphQLFieldDefinition fieldDefinition, - TraverserContext context) { + public TraversalControl visitGraphQLFieldDefinition( + GraphQLFieldDefinition fieldDefinition, TraverserContext context) { GraphQLCodeRegistry.Builder codeRegistry = context.getVarFromParents(GraphQLCodeRegistry.Builder.class); GraphQLFieldsContainer parent = (GraphQLFieldsContainer) context.getParentNode(); DataFetcher dataFetcher = codeRegistry.getDataFetcher(parent, fieldDefinition); - if (dataFetcher.getClass().getPackage().getName().startsWith("graphql.")) { + if (skipDataFetcher(dataFetcher)) { return TraversalControl.CONTINUE; } @@ -125,6 +125,14 @@ final class ContextDataFetcherDecorator implements DataFetcher { codeRegistry.dataFetcher(parent, fieldDefinition, dataFetcher); return TraversalControl.CONTINUE; } + + private boolean skipDataFetcher(DataFetcher dataFetcher) { + Class type = dataFetcher.getClass(); + if (type.getPackage().getName().startsWith("graphql.")) { + return !type.getSimpleName().startsWith("DataFetcherFactories"); + } + return false; + } }; } diff --git a/spring-graphql/src/test/java/org/springframework/graphql/execution/ContextDataFetcherDecoratorTests.java b/spring-graphql/src/test/java/org/springframework/graphql/execution/ContextDataFetcherDecoratorTests.java index 56befead..43fb90ae 100644 --- a/spring-graphql/src/test/java/org/springframework/graphql/execution/ContextDataFetcherDecoratorTests.java +++ b/spring-graphql/src/test/java/org/springframework/graphql/execution/ContextDataFetcherDecoratorTests.java @@ -19,12 +19,19 @@ package org.springframework.graphql.execution; import java.time.Duration; import java.util.Collections; import java.util.List; +import java.util.concurrent.CompletableFuture; +import java.util.function.BiConsumer; import graphql.ExecutionInput; import graphql.ExecutionResult; import graphql.GraphQL; import graphql.GraphQLError; import graphql.GraphqlErrorBuilder; +import graphql.schema.DataFetcher; +import graphql.schema.DataFetcherFactories; +import graphql.schema.GraphQLFieldDefinition; +import graphql.schema.idl.SchemaDirectiveWiring; +import graphql.schema.idl.SchemaDirectiveWiringEnvironment; import org.junit.jupiter.api.Test; import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; @@ -42,10 +49,18 @@ import static org.assertj.core.api.Assertions.assertThat; * Tests for {@link ContextDataFetcherDecorator}. * @author Rossen Stoyanchev */ +@SuppressWarnings("ReactiveStreamsUnusedPublisher") public class ContextDataFetcherDecoratorTests { - private static final String SCHEMA_CONTENT = - "type Query { greeting: String, greetings: [String] } type Subscription { greetings: String }"; + private static final String SCHEMA_CONTENT = "" + + "directive @UpperCase on FIELD_DEFINITION " + + "type Query { " + + " greeting: String @UpperCase, " + + " greetings: [String] " + + "} " + + "type Subscription { " + + " greetings: String " + + "}"; @Test @@ -112,7 +127,7 @@ public class ContextDataFetcherDecoratorTests { } @Test - void fluxDataFetcherSubscriptionThrowException() throws Exception { + void fluxDataFetcherSubscriptionThrowingException() throws Exception { SubscriptionExceptionResolver resolver = SubscriptionExceptionResolver.forSingleError(exception -> @@ -177,4 +192,51 @@ public class ContextDataFetcherDecoratorTests { } } + @Test // gh-440 + void dataFetcherDecoratedWithDataFetcherFactories() { + + SchemaDirectiveWiring directiveWiring = new SchemaDirectiveWiring() { + + @SuppressWarnings("unchecked") + @Override + public GraphQLFieldDefinition onField(SchemaDirectiveWiringEnvironment env) { + if (env.getDirective("UpperCase") != null) { + return env.setFieldDataFetcher(DataFetcherFactories.wrapDataFetcher( + env.getFieldDataFetcher(), + ((dataFetchingEnv, value) -> { + if (value instanceof String) { + return ((String) value).toUpperCase(); + } + else if (value instanceof Mono) { + return ((Mono) value).map(String::toUpperCase); + } + else { + throw new IllegalArgumentException(); + } + }))); + } + else { + return env.getElement(); + } + } + }; + + BiConsumer> tester = (schemaDirectiveWiring, dataFetcher) -> { + + GraphQL graphQl = GraphQlSetup.schemaContent(SCHEMA_CONTENT) + .queryFetcher("greeting", dataFetcher) + .runtimeWiring(builder -> builder.directiveWiring(directiveWiring)) + .toGraphQl(); + + ExecutionInput input = ExecutionInput.newExecutionInput().query("{ greeting }").build(); + Mono resultMono = Mono.fromFuture(graphQl.executeAsync(input)); + + String greeting = ResponseHelper.forResult(resultMono).toEntity("greeting", String.class); + assertThat(greeting).isEqualTo("HELLO"); + }; + + tester.accept(directiveWiring, env -> CompletableFuture.completedFuture("hello")); + tester.accept(directiveWiring, env -> Mono.just("hello")); + } + } From a7a78d9c6ccc02880cdfdf1cbde896a0035bcd76 Mon Sep 17 00:00:00 2001 From: rstoyanchev Date: Fri, 9 Sep 2022 11:14:08 +0100 Subject: [PATCH 2/3] Do not skip a graphql.validation wrapped DataFetcher Closes gh-479 --- .../ContextDataFetcherDecorator.java | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/spring-graphql/src/main/java/org/springframework/graphql/execution/ContextDataFetcherDecorator.java b/spring-graphql/src/main/java/org/springframework/graphql/execution/ContextDataFetcherDecorator.java index 06696d99..f69264c6 100644 --- a/spring-graphql/src/main/java/org/springframework/graphql/execution/ContextDataFetcherDecorator.java +++ b/spring-graphql/src/main/java/org/springframework/graphql/execution/ContextDataFetcherDecorator.java @@ -116,22 +116,23 @@ final class ContextDataFetcherDecorator implements DataFetcher { GraphQLFieldsContainer parent = (GraphQLFieldsContainer) context.getParentNode(); DataFetcher dataFetcher = codeRegistry.getDataFetcher(parent, fieldDefinition); - if (skipDataFetcher(dataFetcher)) { - return TraversalControl.CONTINUE; + if (applyDecorator(dataFetcher)) { + boolean handlesSubscription = parent.getName().equals("Subscription"); + dataFetcher = new ContextDataFetcherDecorator(dataFetcher, handlesSubscription, compositeResolver); + codeRegistry.dataFetcher(parent, fieldDefinition, dataFetcher); } - boolean handlesSubscription = parent.getName().equals("Subscription"); - dataFetcher = new ContextDataFetcherDecorator(dataFetcher, handlesSubscription, compositeResolver); - codeRegistry.dataFetcher(parent, fieldDefinition, dataFetcher); return TraversalControl.CONTINUE; } - private boolean skipDataFetcher(DataFetcher dataFetcher) { + private boolean applyDecorator(DataFetcher dataFetcher) { Class type = dataFetcher.getClass(); - if (type.getPackage().getName().startsWith("graphql.")) { - return !type.getSimpleName().startsWith("DataFetcherFactories"); + String packageName = type.getPackage().getName(); + if (packageName.startsWith("graphql.")) { + return (type.getSimpleName().startsWith("DataFetcherFactories") || + packageName.startsWith("graphql.validation")); } - return false; + return true; } }; } From a1165051eb53890269df33ccec402b36646e07cf Mon Sep 17 00:00:00 2001 From: rstoyanchev Date: Fri, 9 Sep 2022 12:25:10 +0100 Subject: [PATCH 3/3] Support method parameter annotations on interface Closes gh-480 --- .../graphql/data/method/HandlerMethod.java | 78 ++++++++++++++++++- .../DataFetcherHandlerMethodTests.java | 46 ++++++++++- 2 files changed, 119 insertions(+), 5 deletions(-) diff --git a/spring-graphql/src/main/java/org/springframework/graphql/data/method/HandlerMethod.java b/spring-graphql/src/main/java/org/springframework/graphql/data/method/HandlerMethod.java index e56eb4f3..9a428f90 100644 --- a/spring-graphql/src/main/java/org/springframework/graphql/data/method/HandlerMethod.java +++ b/spring-graphql/src/main/java/org/springframework/graphql/data/method/HandlerMethod.java @@ -17,6 +17,9 @@ package org.springframework.graphql.data.method; import java.lang.annotation.Annotation; import java.lang.reflect.Method; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; import java.util.stream.Collectors; import java.util.stream.IntStream; @@ -26,6 +29,7 @@ import org.apache.commons.logging.LogFactory; import org.springframework.beans.factory.BeanFactory; import org.springframework.core.BridgeMethodResolver; import org.springframework.core.MethodParameter; +import org.springframework.core.ResolvableType; import org.springframework.core.annotation.AnnotatedElementUtils; import org.springframework.core.annotation.SynthesizingMethodParameter; import org.springframework.lang.Nullable; @@ -67,6 +71,9 @@ public class HandlerMethod { private final MethodParameter[] parameters; + @Nullable + private volatile List interfaceParameterAnnotations; + /** * Constructor with a handler instance and a method. @@ -241,6 +248,40 @@ public class HandlerMethod { return getBeanType().getSimpleName() + "#" + this.method.getName() + "[" + args + " args]"; } + private List getInterfaceParameterAnnotations() { + List parameterAnnotations = this.interfaceParameterAnnotations; + if (parameterAnnotations == null) { + parameterAnnotations = new ArrayList<>(); + for (Class ifc : ClassUtils.getAllInterfacesForClassAsSet(this.method.getDeclaringClass())) { + for (Method candidate : ifc.getMethods()) { + if (isOverrideFor(candidate)) { + parameterAnnotations.add(candidate.getParameterAnnotations()); + } + } + } + this.interfaceParameterAnnotations = parameterAnnotations; + } + return parameterAnnotations; + } + + private boolean isOverrideFor(Method candidate) { + if (!candidate.getName().equals(this.method.getName()) || + candidate.getParameterCount() != this.method.getParameterCount()) { + return false; + } + Class[] paramTypes = this.method.getParameterTypes(); + if (Arrays.equals(candidate.getParameterTypes(), paramTypes)) { + return true; + } + for (int i = 0; i < paramTypes.length; i++) { + if (paramTypes[i] != + ResolvableType.forMethodParameter(candidate, i, this.method.getDeclaringClass()).resolve()) { + return false; + } + } + return true; + } + @Override public boolean equals(@Nullable Object other) { @@ -323,6 +364,9 @@ public class HandlerMethod { */ protected class HandlerMethodParameter extends SynthesizingMethodParameter { + @Nullable + private volatile Annotation[] combinedAnnotations; + public HandlerMethodParameter(int index) { super(HandlerMethod.this.bridgedMethod, index); } @@ -347,8 +391,38 @@ public class HandlerMethod { } @Override - public HandlerMethodParameter clone() { - return new HandlerMethodParameter(this); + public Annotation[] getParameterAnnotations() { + Annotation[] anns = this.combinedAnnotations; + if (anns == null) { + anns = super.getParameterAnnotations(); + int index = getParameterIndex(); + if (index >= 0) { + for (Annotation[][] ifcAnns : getInterfaceParameterAnnotations()) { + if (index < ifcAnns.length) { + Annotation[] paramAnns = ifcAnns[index]; + if (paramAnns.length > 0) { + List merged = new ArrayList<>(anns.length + paramAnns.length); + merged.addAll(Arrays.asList(anns)); + for (Annotation paramAnn : paramAnns) { + boolean existingType = false; + for (Annotation ann : anns) { + if (ann.annotationType() == paramAnn.annotationType()) { + existingType = true; + break; + } + } + if (!existingType) { + merged.add(adaptAnnotation(paramAnn)); + } + } + anns = merged.toArray(new Annotation[0]); + } + } + } + } + this.combinedAnnotations = anns; + } + return anns; } } diff --git a/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/DataFetcherHandlerMethodTests.java b/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/DataFetcherHandlerMethodTests.java index 7ccade17..5a4041ba 100644 --- a/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/DataFetcherHandlerMethodTests.java +++ b/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/DataFetcherHandlerMethodTests.java @@ -16,6 +16,8 @@ package org.springframework.graphql.data.method.annotation.support; +import java.lang.reflect.Method; +import java.util.Collections; import java.util.concurrent.Callable; import java.util.concurrent.CompletableFuture; @@ -26,10 +28,14 @@ import org.junit.jupiter.api.Test; import org.mockito.Mockito; import org.springframework.core.task.SimpleAsyncTaskExecutor; +import org.springframework.graphql.data.GraphQlArgumentBinder; import org.springframework.graphql.data.method.HandlerMethod; import org.springframework.graphql.data.method.HandlerMethodArgumentResolver; import org.springframework.graphql.data.method.HandlerMethodArgumentResolverComposite; +import org.springframework.graphql.data.method.annotation.Argument; +import org.springframework.graphql.data.method.annotation.QueryMapping; import org.springframework.lang.Nullable; +import org.springframework.util.ClassUtils; import static org.assertj.core.api.Assertions.assertThat; @@ -40,6 +46,22 @@ import static org.assertj.core.api.Assertions.assertThat; */ public class DataFetcherHandlerMethodTests { + @Test + void annotatedMethodsOnInterface() { + + HandlerMethodArgumentResolverComposite resolvers = new HandlerMethodArgumentResolverComposite(); + resolvers.addResolver(new ArgumentMethodArgumentResolver(new GraphQlArgumentBinder())); + + DataFetcherHandlerMethod handlerMethod = new DataFetcherHandlerMethod( + handlerMethodFor(new TestController(), "hello"), resolvers, null, null, false); + + Object result = handlerMethod.invoke( + DataFetchingEnvironmentImpl.newDataFetchingEnvironment() + .arguments(Collections.singletonMap("name", "Neil")) + .build()); + + assertThat(result).isEqualTo("Hello, Neil"); + } @Test void callableReturnValue() throws Exception { @@ -48,8 +70,8 @@ public class DataFetcherHandlerMethodTests { resolvers.addResolver(Mockito.mock(HandlerMethodArgumentResolver.class)); DataFetcherHandlerMethod handlerMethod = new DataFetcherHandlerMethod( - new HandlerMethod(new TestController(), TestController.class.getMethod("handleAndReturnCallable")), - resolvers, null, new SimpleAsyncTaskExecutor(), false); + handlerMethodFor(new TestController(), "handleAndReturnCallable"), resolvers, null, + new SimpleAsyncTaskExecutor(), false); GraphQLContext graphQLContext = new GraphQLContext.Builder().build(); @@ -64,8 +86,26 @@ public class DataFetcherHandlerMethodTests { assertThat(future.get()).isEqualTo("A"); } + private static HandlerMethod handlerMethodFor(Object controller, String methodName) { + Method method = ClassUtils.getMethod(controller.getClass(), methodName, (Class[]) null); + return new HandlerMethod(controller, method); + } - private static class TestController { + + interface TestInterface { + + @QueryMapping + String hello(@Argument String name); + + } + + @SuppressWarnings("unused") + private static class TestController implements TestInterface { + + @Override + public String hello(String name) { + return "Hello, " + name; + } @Nullable public Callable handleAndReturnCallable() {