From 97c7ffa6580d818f66ff6add167ef1f2b3e42445 Mon Sep 17 00:00:00 2001 From: Victor Levasseur Date: Wed, 10 Nov 2021 23:17:32 +0100 Subject: [PATCH] DefaultDgsFederationResolver now handles failed completion stages returned from entity fetchers properly --- .gitignore | 2 + .../DefaultDgsFederationResolver.kt | 84 +++++++++++-------- .../graphql/dgs/DgsFederationResolverTest.kt | 30 +++++++ 3 files changed, 82 insertions(+), 34 deletions(-) diff --git a/.gitignore b/.gitignore index 5195919b6..b662672e0 100644 --- a/.gitignore +++ b/.gitignore @@ -23,6 +23,8 @@ build .idea/**/dynamic.xml .idea/**/uiDesigner.xml .idea/**/dbnavigator.xml +.idea/**/runConfigurations.xml +.idea/**/aws.xml # Gradle .idea/**/gradle.xml diff --git a/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/federation/DefaultDgsFederationResolver.kt b/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/federation/DefaultDgsFederationResolver.kt index 73c0630d2..2f685c987 100644 --- a/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/federation/DefaultDgsFederationResolver.kt +++ b/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/federation/DefaultDgsFederationResolver.kt @@ -24,11 +24,14 @@ import com.netflix.graphql.dgs.exceptions.InvalidDgsEntityFetcher import com.netflix.graphql.dgs.exceptions.MissingDgsEntityFetcherException import com.netflix.graphql.dgs.internal.DgsSchemaProvider import com.netflix.graphql.types.errors.TypedGraphQLError -import graphql.GraphQLError -import graphql.execution.* +import graphql.execution.DataFetcherExceptionHandler +import graphql.execution.DataFetcherExceptionHandlerParameters +import graphql.execution.DataFetcherResult +import graphql.execution.ResultPath import graphql.schema.DataFetcher import graphql.schema.DataFetchingEnvironment import graphql.schema.TypeResolver +import org.dataloader.Try import org.slf4j.Logger import org.slf4j.LoggerFactory import org.springframework.beans.factory.annotation.Autowired @@ -46,7 +49,10 @@ open class DefaultDgsFederationResolver() : * This is the most common use case. * The default constructor is used to extend the DefaultDgsFederationResolver. In that case injection is used to provide the schemaProvider. */ - constructor(providedDgsSchemaProvider: DgsSchemaProvider, dataFetcherExceptionHandler: Optional) : this() { + constructor( + providedDgsSchemaProvider: DgsSchemaProvider, + dataFetcherExceptionHandler: Optional + ) : this() { dgsSchemaProvider = providedDgsSchemaProvider dgsExceptionHandler = dataFetcherExceptionHandler } @@ -68,10 +74,9 @@ open class DefaultDgsFederationResolver() : } private fun dgsEntityFetchers(env: DataFetchingEnvironment): CompletableFuture>> { - val errorsList = mutableListOf() val resultList = env.getArgument>>(_Entity.argumentName) .map { values -> - try { + Try.tryCall { val typename = values["__typename"] ?: throw RuntimeException("__typename missing from arguments in federated query") val fetcher = dgsSchemaProvider.entityFetchers[typename] @@ -96,39 +101,50 @@ open class DefaultDgsFederationResolver() : } else { CompletableFuture.completedFuture(result) } - } catch (e: Exception) { - if (e is InvocationTargetException && e.targetException != null) { - if (dgsExceptionHandler.isPresent) { - val res = dgsExceptionHandler.get().onException( - DataFetcherExceptionHandlerParameters.newExceptionParameters() - .dataFetchingEnvironment(env).exception(e.targetException).build() - ) - res.errors.forEach { errorsList.add(it) } - } else { - errorsList.add( - TypedGraphQLError.newInternalErrorBuilder() - .message("%s: %s", e.targetException::class.java.name, e.targetException.message) - .path(ResultPath.parse("/_entities")).build() - ) - } - } else { - errorsList.add( - TypedGraphQLError.newInternalErrorBuilder().message("%s: %s", e::class.java.name, e.message) - .path(ResultPath.parse("/_entities")).build() - ) - } - CompletableFuture.completedFuture(null) } + .map { tryFuture -> Try.tryFuture(tryFuture) } + .recover { exception -> CompletableFuture.completedFuture(Try.failed(exception)) } + .get() } return CompletableFuture.allOf(*resultList.toTypedArray()).thenApply { - DataFetcherResult.newResult>().data( - resultList.asSequence() - .map { cf -> cf.join() } - .flatMap { r -> if (r is Collection<*>) r.asSequence() else sequenceOf(r) } - .toList() - ) - .errors(errorsList) + DataFetcherResult.newResult>() + .data( + resultList.asSequence() + .map { cf -> cf.join() } + .map { tryResult -> tryResult.orElse(null) } + .flatMap { r -> if (r is Collection<*>) r.asSequence() else sequenceOf(r) } + .toList() + ) + .errors( + resultList.asSequence() + .map { cf -> cf.join() } + .filter { tryResult -> tryResult.isFailure } + .map { tryResult -> tryResult.throwable } + .flatMap { e -> + if (e is InvocationTargetException && e.targetException != null) { + if (dgsExceptionHandler.isPresent) { + val res = dgsExceptionHandler.get().onException( + DataFetcherExceptionHandlerParameters.newExceptionParameters() + .dataFetchingEnvironment(env).exception(e.targetException).build() + ) + res.errors.asSequence() + } else { + sequenceOf( + TypedGraphQLError.newInternalErrorBuilder() + .message("%s: %s", e.targetException::class.java.name, e.targetException.message) + .path(ResultPath.parse("/_entities")).build() + ) + } + } else { + sequenceOf( + TypedGraphQLError.newInternalErrorBuilder().message("%s: %s", e::class.java.name, e.message) + .path(ResultPath.parse("/_entities")).build() + ) + } + } + .toList() + ) .build() } } diff --git a/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/DgsFederationResolverTest.kt b/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/DgsFederationResolverTest.kt index 93af1deff..79ada4a7b 100644 --- a/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/DgsFederationResolverTest.kt +++ b/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/DgsFederationResolverTest.kt @@ -237,6 +237,36 @@ class DgsFederationResolverTest { assertThat(result.get().errors[0].message).contains("DgsInvalidInputArgumentException") } + @Test + fun dgsEntityFetcherWithFailedCompletableFuture() { + + val movieEntityFetcher = object { + @DgsEntityFetcher(name = "Movie") + fun movieEntityFetcher(values: Map): CompletableFuture { + return CompletableFuture.supplyAsync { + if (values["movieId"] == "invalid") { + throw DgsInvalidInputArgumentException("Invalid input argument exception") + } + + Movie(values["movieId"].toString()) + } + } + } + every { applicationContextMock.getBeansWithAnnotation(DgsScalar::class.java) } returns emptyMap() + every { applicationContextMock.getBeansWithAnnotation(DgsDirective::class.java) } returns emptyMap() + every { applicationContextMock.getBeansWithAnnotation(DgsComponent::class.java) } returns mapOf(Pair("MovieEntityFetcher", movieEntityFetcher)) + dgsSchemaProvider.schema("""type Query {}""") + + val arguments = mapOf(Pair(_Entity.argumentName, listOf(mapOf(Pair("__typename", "Movie"), Pair("movieId", "invalid"))))) + val executionStepInfo = ExecutionStepInfo.newExecutionStepInfo().path(ResultPath.parse("/_entities")).type(GraphQLList.list(GraphQLUnionType.newUnionType().name("Entity").possibleTypes(GraphQLObjectType.newObject().name("Movie").build()).build())).build() + val dataFetchingEnvironment = DgsDataFetchingEnvironment(DataFetchingEnvironmentImpl.newDataFetchingEnvironment().arguments(arguments).executionStepInfo(executionStepInfo).build()) + val result = (DefaultDgsFederationResolver(dgsSchemaProvider, Optional.of(dgsExceptionHandler)).entitiesFetcher().get(dataFetchingEnvironment) as CompletableFuture>>) + assertThat(result).isNotNull + assertThat(result.get().data.size).isEqualTo(1) + assertThat(result.get().errors.size).isEqualTo(1) + assertThat(result.get().errors[0].message).contains("DgsInvalidInputArgumentException") + } + @Test fun dgsEntityFetcherWithIllegalArgumentException() {