Polishing in SelfDescribingDataFetcher

Author: rstoyanchev

Diff for: spring-graphql/src/main/java/org/springframework/graphql/data/method/annotation/support/AnnotatedControllerConfigurer.java

@@ -464,13 +464,12 @@ static class SchemaMappingDataFetcher implements SelfDescribingDataFetcher<Objec
 			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 String getDescription() {
 
 		@Override
 		public ResolvableType getReturnType() {
-			return ResolvableType.forMethodReturnType(this.mappingInfo.getHandlerMethod().getMethod());
+			return ResolvableType.forMethodReturnType(getHandlerMethod().getMethod());
+		}
+
+		HandlerMethod getHandlerMethod() {
+			return this.mappingInfo.getHandlerMethod();
 		}
 
 		@Override
@@ -493,19 +496,17 @@ public Map<String, ResolvableType> getArguments() {
 			Predicate<MethodParameter> 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(
 							ArgumentMethodArgumentResolver::getArgumentName,
 							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 @@ private <T> Publisher<T> handleSubscriptionError(
 					.switchIfEmpty(Mono.error(ex));
 		}
 
-		@Override
-		public boolean isBatchLoading() {
-			return this.usesDataLoader;
-		}
-
 		@Override
 		public String toString() {
 			return getDescription();
@@ -614,7 +610,7 @@ public Object get(DataFetchingEnvironment env) {
 		}
 
 		@Override
-		public boolean isBatchLoading() {
+		public boolean usesDataLoader() {
 			return true;
 		}
 

Diff for: spring-graphql/src/main/java/org/springframework/graphql/execution/ContextDataFetcherDecorator.java

@@ -150,17 +150,6 @@ static GraphQLTypeVisitor createVisitor(List<SubscriptionExceptionResolver> reso
 		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 @@ public TraversalControl visitGraphQLFieldDefinition(
 			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 @@ private boolean applyDecorator(DataFetcher<?> 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<Object> {
 
-		private final SelfDescribingDataFetcher<?> selfDescribingDataFetcher;
+	private static final class SelfDescribingContextDataFetcherDecorator extends ContextDataFetcherDecorator
+			implements SelfDescribingDataFetcher<Object> {
 
-		private SelfDescribingDecorator(
-				SelfDescribingDataFetcher<?> delegate, boolean subscription,
-				SubscriptionExceptionResolver subscriptionExceptionResolver) {
-			super(delegate, subscription, subscriptionExceptionResolver);
-			this.selfDescribingDataFetcher = delegate;
+		private final SelfDescribingDataFetcher<?> delegate;
+
+		private SelfDescribingContextDataFetcherDecorator(
+				SelfDescribingDataFetcher<?> delegate, boolean subscriptionType,
+				SubscriptionExceptionResolver exceptionResolver) {
+
+			super(delegate, subscriptionType, exceptionResolver);
+			this.delegate = delegate;
 		}
 
 		@Override
-		public boolean isBatchLoading() {
-			return this.selfDescribingDataFetcher.isBatchLoading();
+		public String getDescription() {
+			return this.delegate.getDescription();
 		}
 
 		@Override
-		public Map<String, ResolvableType> getArguments() {
-			return this.selfDescribingDataFetcher.getArguments();
+		public ResolvableType getReturnType() {
+			return this.delegate.getReturnType();
 		}
 
 		@Override
-		public ResolvableType getReturnType() {
-			return this.selfDescribingDataFetcher.getReturnType();
+		public Map<String, ResolvableType> getArguments() {
+			return this.delegate.getArguments();
 		}
 
 		@Override
-		public String getDescription() {
-			return this.selfDescribingDataFetcher.getDescription();
+		public boolean usesDataLoader() {
+			return this.delegate.usesDataLoader();
 		}
 	}
 

Diff for: spring-graphql/src/main/java/org/springframework/graphql/execution/SelfDescribingDataFetcher.java

@@ -60,11 +60,13 @@ default Map<String, ResolvableType> getArguments() {
 	}
 
 	/**
-	 * 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 for: spring-graphql/src/main/java/org/springframework/graphql/observation/GraphQlObservationInstrumentation.java

@@ -187,7 +187,7 @@ public DataFetcher<?> instrumentDataFetcher(DataFetcher<?> dataFetcher,
 				&& 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 for: spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/BatchMappingDetectionTests.java

@@ -83,7 +83,7 @@ void dataFetchersMarkedAsBatchLoading() {
 				initRuntimeWiringBuilder(BookController.class).build().getDataFetchers();
 		assertThat(dataFetcherMap.get("Book").values()).allMatch(dataFetcher ->
 				dataFetcher instanceof SelfDescribingDataFetcher<?> selfDescribingDataFetcher
-						&& selfDescribingDataFetcher.isBatchLoading());
+						&& selfDescribingDataFetcher.usesDataLoader());
 	}
 
 	@Test

Diff for: spring-graphql/src/test/java/org/springframework/graphql/data/method/annotation/support/SchemaMappingDetectionTests.java

@@ -91,13 +91,13 @@ void batchLoadingDataFetchers() {
 		Map<String, DataFetcher> 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 for: spring-graphql/src/test/java/org/springframework/graphql/observation/GraphQlObservationInstrumentationTests.java

@@ -419,7 +419,7 @@ void shouldRecordBatchLoadingAsSingleObservation() {
 	static class AuthorBatchLoadingDataFetcher implements SelfDescribingDataFetcher<CompletableFuture<Author>> {
 
 		@Override
-		public boolean isBatchLoading() {
+		public boolean usesDataLoader() {
 			return true;
 		}