From 43108f7fed16c4018096c4d1c391c7836c6059a7 Mon Sep 17 00:00:00 2001 From: Bernardo Gomez Palacio Date: Thu, 11 Nov 2021 16:52:14 -0800 Subject: [PATCH] Missing the typename in a federated query should return a BadRequest When a _Federated Request_ is missing the `_typename` we shouldn't return an exception that can properly be matched to a _client generated bad request_. --- .../DefaultDataFetcherExceptionHandler.kt | 9 +++-- .../dgs/exceptions/DgsBadRequestException.kt | 2 +- .../MissingFederatedQueryArgument.kt | 19 +++++++++++ .../DefaultDgsFederationResolver.kt | 33 ++++++++++++------- .../graphql/dgs/DgsFederationResolverTest.kt | 3 +- 5 files changed, 49 insertions(+), 17 deletions(-) create mode 100644 graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/exceptions/MissingFederatedQueryArgument.kt diff --git a/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/exceptions/DefaultDataFetcherExceptionHandler.kt b/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/exceptions/DefaultDataFetcherExceptionHandler.kt index 39c806d40..0a68410ad 100644 --- a/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/exceptions/DefaultDataFetcherExceptionHandler.kt +++ b/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/exceptions/DefaultDataFetcherExceptionHandler.kt @@ -40,13 +40,16 @@ class DefaultDataFetcherExceptionHandler : DataFetcherExceptionHandler { .message("%s: %s", exception::class.java.name, exception.message) .path(handlerParameters.path).build() } else if (exception is DgsEntityNotFoundException) { - TypedGraphQLError.newNotFoundBuilder().message("%s: %s", exception::class.java.name, exception.message) + TypedGraphQLError.newNotFoundBuilder() + .message("%s: %s", exception::class.java.name, exception.message) .path(handlerParameters.path).build() } else if (exception is DgsBadRequestException) { - TypedGraphQLError.newBadRequestBuilder().message("%s: %s", exception::class.java.name, exception.message) + TypedGraphQLError.newBadRequestBuilder() + .message("%s: %s", exception::class.java.name, exception.message) .path(handlerParameters.path).build() } else { - TypedGraphQLError.newInternalErrorBuilder().message("%s: %s", exception::class.java.name, exception.message) + TypedGraphQLError.newInternalErrorBuilder() + .message("%s: %s", exception::class.java.name, exception.message) .path(handlerParameters.path).build() } diff --git a/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/exceptions/DgsBadRequestException.kt b/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/exceptions/DgsBadRequestException.kt index dc29dad98..55dd4a9f8 100644 --- a/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/exceptions/DgsBadRequestException.kt +++ b/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/exceptions/DgsBadRequestException.kt @@ -16,4 +16,4 @@ package com.netflix.graphql.dgs.exceptions -class DgsBadRequestException(override val message: String = "Malformed or incorrect request") : RuntimeException(message) +open class DgsBadRequestException(override val message: String = "Malformed or incorrect request") : RuntimeException(message) diff --git a/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/exceptions/MissingFederatedQueryArgument.kt b/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/exceptions/MissingFederatedQueryArgument.kt new file mode 100644 index 000000000..7bfa0d045 --- /dev/null +++ b/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/exceptions/MissingFederatedQueryArgument.kt @@ -0,0 +1,19 @@ +/* + * Copyright 2021 Netflix, Inc. + * + * 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 + * + * http://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 com.netflix.graphql.dgs.exceptions + +class MissingFederatedQueryArgument(vararg fields: String) : DgsBadRequestException("The federated query is missing field(s) ${fields.joinToString()}") 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 2f685c987..ad97de933 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 @@ -22,6 +22,7 @@ import com.netflix.graphql.dgs.DgsDataFetchingEnvironment import com.netflix.graphql.dgs.DgsFederationResolver import com.netflix.graphql.dgs.exceptions.InvalidDgsEntityFetcher import com.netflix.graphql.dgs.exceptions.MissingDgsEntityFetcherException +import com.netflix.graphql.dgs.exceptions.MissingFederatedQueryArgument import com.netflix.graphql.dgs.internal.DgsSchemaProvider import com.netflix.graphql.types.errors.TypedGraphQLError import graphql.execution.DataFetcherExceptionHandler @@ -78,7 +79,7 @@ open class DefaultDgsFederationResolver() : .map { values -> Try.tryCall { val typename = values["__typename"] - ?: throw RuntimeException("__typename missing from arguments in federated query") + ?: throw MissingFederatedQueryArgument("__typename") val fetcher = dgsSchemaProvider.entityFetchers[typename] ?: throw MissingDgsEntityFetcherException(typename.toString()) @@ -108,38 +109,46 @@ open class DefaultDgsFederationResolver() : } return CompletableFuture.allOf(*resultList.toTypedArray()).thenApply { + val trySequence = resultList.asSequence().map { it.join() } DataFetcherResult.newResult>() .data( - resultList.asSequence() - .map { cf -> cf.join() } + trySequence .map { tryResult -> tryResult.orElse(null) } .flatMap { r -> if (r is Collection<*>) r.asSequence() else sequenceOf(r) } .toList() ) .errors( - resultList.asSequence() - .map { cf -> cf.join() } + trySequence .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() + val res = dgsExceptionHandler.get().handleException( + DataFetcherExceptionHandlerParameters + .newExceptionParameters() + .dataFetchingEnvironment(env) + .exception(e.targetException) + .build() ) - res.errors.asSequence() + res.join().errors.asSequence() } else { sequenceOf( TypedGraphQLError.newInternalErrorBuilder() - .message("%s: %s", e.targetException::class.java.name, e.targetException.message) + .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() + TypedGraphQLError.newInternalErrorBuilder() + .message("%s: %s", e::class.java.name, e.message) + .path(ResultPath.parse("/_entities")) + .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 79ada4a7b..38372eaa3 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 @@ -142,7 +142,8 @@ class DgsFederationResolverTest { assertThat(result).isNotNull assertThat(result.get().data.size).isEqualTo(1) assertThat(result.get().errors.size).isEqualTo(1) - assertThat(result.get().errors[0].message).contains("RuntimeException") + assertThat(result.get().errors.first().message) + .endsWith("MissingFederatedQueryArgument: The federated query is missing field(s) __typename") } @Test