From cea4e080110ab80f9f81562da5a886defc8fd939 Mon Sep 17 00:00:00 2001 From: rstoyanchev Date: Thu, 17 Apr 2025 14:48:23 +0100 Subject: [PATCH] Polishing in SelfDescribingDataFetcher --- .../AnnotatedControllerConfigurer.java | 32 ++++----- .../ContextDataFetcherDecorator.java | 68 +++++++++---------- .../execution/SelfDescribingDataFetcher.java | 8 ++- .../GraphQlObservationInstrumentation.java | 2 +- .../support/BatchMappingDetectionTests.java | 2 +- .../support/SchemaMappingDetectionTests.java | 4 +- ...raphQlObservationInstrumentationTests.java | 2 +- 7 files changed, 58 insertions(+), 60 deletions(-) diff --git a/spring-graphql/src/main/java/org/springframework/graphql/data/method/annotation/support/AnnotatedControllerConfigurer.java b/spring-graphql/src/main/java/org/springframework/graphql/data/method/annotation/support/AnnotatedControllerConfigurer.java index ed2ec224..1df1b6ec 100644 --- a/spring-graphql/src/main/java/org/springframework/graphql/data/method/annotation/support/AnnotatedControllerConfigurer.java +++ b/spring-graphql/src/main/java/org/springframework/graphql/data/method/annotation/support/AnnotatedControllerConfigurer.java @@ -464,13 +464,12 @@ public class AnnotatedControllerConfigurer this.executor = executor; this.invokeAsync = invokeAsync; this.subscription = this.mappingInfo.getCoordinates().getTypeName().equalsIgnoreCase("Subscription"); - this.usesDataLoader = hasDataLoaderParameter(); + this.usesDataLoader = hasDataLoaderParameter(info.getHandlerMethod()); } - private boolean hasDataLoaderParameter() { - Method handlerMethod = this.mappingInfo.getHandlerMethod().getMethod(); - for (Class parameterType : handlerMethod.getParameterTypes()) { - if (DataLoader.class.equals(parameterType)) { + private static boolean hasDataLoaderParameter(HandlerMethod method) { + for (MethodParameter type : method.getMethodParameters()) { + if (DataLoader.class.equals(type.getParameterType())) { return true; } } @@ -484,7 +483,11 @@ public class AnnotatedControllerConfigurer @Override public ResolvableType getReturnType() { - return ResolvableType.forMethodReturnType(this.mappingInfo.getHandlerMethod().getMethod()); + return ResolvableType.forMethodReturnType(getHandlerMethod().getMethod()); + } + + HandlerMethod getHandlerMethod() { + return this.mappingInfo.getHandlerMethod(); } @Override @@ -493,7 +496,7 @@ public class AnnotatedControllerConfigurer Predicate argumentPredicate = (p) -> (p.getParameterAnnotation(Argument.class) != null || p.getParameterType() == ArgumentValue.class); - return Arrays.stream(this.mappingInfo.getHandlerMethod().getMethodParameters()) + return Arrays.stream(getHandlerMethod().getMethodParameters()) .filter(argumentPredicate) .peek((p) -> p.initParameterNameDiscovery(parameterNameDiscoverer)) .collect(Collectors.toMap( @@ -501,11 +504,9 @@ public class AnnotatedControllerConfigurer ResolvableType::forMethodParameter)); } - /** - * Return the {@link HandlerMethod} used to fetch data. - */ - HandlerMethod getHandlerMethod() { - return this.mappingInfo.getHandlerMethod(); + @Override + public boolean usesDataLoader() { + return this.usesDataLoader; } @Override @@ -564,11 +565,6 @@ public class AnnotatedControllerConfigurer .switchIfEmpty(Mono.error(ex)); } - @Override - public boolean isBatchLoading() { - return this.usesDataLoader; - } - @Override public String toString() { return getDescription(); @@ -614,7 +610,7 @@ public class AnnotatedControllerConfigurer } @Override - public boolean isBatchLoading() { + public boolean usesDataLoader() { return true; } 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 800ab5f1..6153a45f 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 @@ -150,17 +150,6 @@ class ContextDataFetcherDecorator implements DataFetcher { return new ContextTypeVisitor(resolvers); } - private static ContextDataFetcherDecorator decorate( - DataFetcher delegate, boolean handlesSubscription, - SubscriptionExceptionResolver subscriptionExceptionResolver) { - if (delegate instanceof SelfDescribingDataFetcher selfDescribingDataFetcher) { - return new SelfDescribingDecorator(selfDescribingDataFetcher, handlesSubscription, subscriptionExceptionResolver); - } - else { - return new ContextDataFetcherDecorator(delegate, handlesSubscription, subscriptionExceptionResolver); - } - } - /** * Type visitor to apply {@link ContextDataFetcherDecorator}. @@ -185,8 +174,8 @@ class ContextDataFetcherDecorator implements DataFetcher { DataFetcher dataFetcher = codeRegistry.getDataFetcher(fieldCoordinates, fieldDefinition); if (applyDecorator(dataFetcher)) { - boolean handlesSubscription = visitorHelper.isSubscriptionType(parent); - dataFetcher = ContextDataFetcherDecorator.decorate(dataFetcher, handlesSubscription, this.exceptionResolver); + boolean subscriptionType = visitorHelper.isSubscriptionType(parent); + dataFetcher = decorate(dataFetcher, subscriptionType, this.exceptionResolver); codeRegistry.dataFetcher(fieldCoordinates, dataFetcher); } @@ -205,37 +194,48 @@ class ContextDataFetcherDecorator implements DataFetcher { } return true; } + + private static ContextDataFetcherDecorator decorate( + DataFetcher dataFetcher, boolean subscriptionType, SubscriptionExceptionResolver exceptionResolver) { + + return ((dataFetcher instanceof SelfDescribingDataFetcher sddf) ? + new SelfDescribingContextDataFetcherDecorator(sddf, subscriptionType, exceptionResolver) : + new ContextDataFetcherDecorator(dataFetcher, subscriptionType, exceptionResolver)); + } } - private static final class SelfDescribingDecorator extends ContextDataFetcherDecorator implements SelfDescribingDataFetcher { - private final SelfDescribingDataFetcher selfDescribingDataFetcher; + private static final class SelfDescribingContextDataFetcherDecorator extends ContextDataFetcherDecorator + implements SelfDescribingDataFetcher { - private SelfDescribingDecorator( - SelfDescribingDataFetcher delegate, boolean subscription, - SubscriptionExceptionResolver subscriptionExceptionResolver) { - super(delegate, subscription, subscriptionExceptionResolver); - this.selfDescribingDataFetcher = delegate; - } + private final SelfDescribingDataFetcher delegate; - @Override - public boolean isBatchLoading() { - return this.selfDescribingDataFetcher.isBatchLoading(); - } + private SelfDescribingContextDataFetcherDecorator( + SelfDescribingDataFetcher delegate, boolean subscriptionType, + SubscriptionExceptionResolver exceptionResolver) { - @Override - public Map getArguments() { - return this.selfDescribingDataFetcher.getArguments(); - } - - @Override - public ResolvableType getReturnType() { - return this.selfDescribingDataFetcher.getReturnType(); + super(delegate, subscriptionType, exceptionResolver); + this.delegate = delegate; } @Override public String getDescription() { - return this.selfDescribingDataFetcher.getDescription(); + return this.delegate.getDescription(); + } + + @Override + public ResolvableType getReturnType() { + return this.delegate.getReturnType(); + } + + @Override + public Map getArguments() { + return this.delegate.getArguments(); + } + + @Override + public boolean usesDataLoader() { + return this.delegate.usesDataLoader(); } } diff --git a/spring-graphql/src/main/java/org/springframework/graphql/execution/SelfDescribingDataFetcher.java b/spring-graphql/src/main/java/org/springframework/graphql/execution/SelfDescribingDataFetcher.java index e43456f1..f1b5f2e5 100644 --- a/spring-graphql/src/main/java/org/springframework/graphql/execution/SelfDescribingDataFetcher.java +++ b/spring-graphql/src/main/java/org/springframework/graphql/execution/SelfDescribingDataFetcher.java @@ -60,11 +60,13 @@ public interface SelfDescribingDataFetcher extends DataFetcher { } /** - * Return whether this {@code DataFetcher} is using batch loading. - * @return {@code true} if the data fetcher is batch loading elements + * Whether this {@code DataFetcher} uses a {@link org.dataloader.DataLoader} + * to return data. This represents a deferred operation that is typically + * repeatable, and a candidate for aggregation from a metrics and tracing + * perspective. * @since 1.4.0 */ - default boolean isBatchLoading() { + default boolean usesDataLoader() { return false; } diff --git a/spring-graphql/src/main/java/org/springframework/graphql/observation/GraphQlObservationInstrumentation.java b/spring-graphql/src/main/java/org/springframework/graphql/observation/GraphQlObservationInstrumentation.java index c8c3bb15..982de281 100644 --- a/spring-graphql/src/main/java/org/springframework/graphql/observation/GraphQlObservationInstrumentation.java +++ b/spring-graphql/src/main/java/org/springframework/graphql/observation/GraphQlObservationInstrumentation.java @@ -187,7 +187,7 @@ public class GraphQlObservationInstrumentation extends SimplePerformantInstrumen && state == RequestObservationInstrumentationState.INSTANCE) { // skip batch loading operations, already instrumented at the dataloader level if (dataFetcher instanceof SelfDescribingDataFetcher selfDescribingDataFetcher - && selfDescribingDataFetcher.isBatchLoading()) { + && selfDescribingDataFetcher.usesDataLoader()) { return dataFetcher; } return (environment) -> { diff --git a/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/BatchMappingDetectionTests.java b/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/BatchMappingDetectionTests.java index 64df5fbb..366aeb1c 100644 --- a/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/BatchMappingDetectionTests.java +++ b/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/BatchMappingDetectionTests.java @@ -83,7 +83,7 @@ class BatchMappingDetectionTests { initRuntimeWiringBuilder(BookController.class).build().getDataFetchers(); assertThat(dataFetcherMap.get("Book").values()).allMatch(dataFetcher -> dataFetcher instanceof SelfDescribingDataFetcher selfDescribingDataFetcher - && selfDescribingDataFetcher.isBatchLoading()); + && selfDescribingDataFetcher.usesDataLoader()); } @Test diff --git a/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/SchemaMappingDetectionTests.java b/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/SchemaMappingDetectionTests.java index e042e041..7517cb31 100644 --- a/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/SchemaMappingDetectionTests.java +++ b/spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/SchemaMappingDetectionTests.java @@ -91,13 +91,13 @@ class SchemaMappingDetectionTests { Map queries = map.get("Book"); assertThat(queries.values()).allMatch(dataFetcher -> dataFetcher instanceof SelfDescribingDataFetcher selfDescribingDataFetcher - && !selfDescribingDataFetcher.isBatchLoading()); + && !selfDescribingDataFetcher.usesDataLoader()); map = initRuntimeWiringBuilder(BatchLoadingController.class).build().getDataFetchers(); queries = map.get("Book"); assertThat(queries.values()).allMatch(dataFetcher -> dataFetcher instanceof SelfDescribingDataFetcher selfDescribingDataFetcher - && selfDescribingDataFetcher.isBatchLoading()); + && selfDescribingDataFetcher.usesDataLoader()); } private RuntimeWiring.Builder initRuntimeWiringBuilder(Class handlerType) { diff --git a/spring-graphql/src/test/java/org/springframework/graphql/observation/GraphQlObservationInstrumentationTests.java b/spring-graphql/src/test/java/org/springframework/graphql/observation/GraphQlObservationInstrumentationTests.java index 49b621be..321e059d 100644 --- a/spring-graphql/src/test/java/org/springframework/graphql/observation/GraphQlObservationInstrumentationTests.java +++ b/spring-graphql/src/test/java/org/springframework/graphql/observation/GraphQlObservationInstrumentationTests.java @@ -419,7 +419,7 @@ class GraphQlObservationInstrumentationTests { static class AuthorBatchLoadingDataFetcher implements SelfDescribingDataFetcher> { @Override - public boolean isBatchLoading() { + public boolean usesDataLoader() { return true; }