From 1db119b29fecc35dfcea44413d5bb9770566e751 Mon Sep 17 00:00:00 2001 From: Kavitha Srinivasan <41588701+srinivasankavitha@users.noreply.github.com> Date: Wed, 23 Aug 2023 13:59:49 -0700 Subject: [PATCH 1/2] Fix entity error path (#1605) * Add the path to index for errors during enity fetcher processing. * Fix lint errors. * Add unit test for different types of entities error * Fix lint errors and small clean up for unit test * Added unit test for resolving with no exception handler when multiple entities of same type DNE. --------- Co-authored-by: Juliana Congote Co-authored-by: Marisela Mendez Co-authored-by: Juliana Congote <42778259+congotej@users.noreply.github.com> --- .../DefaultDgsFederationResolver.kt | 44 ++++- .../dgs/DefaultDgsFederationResolverTest.kt | 167 ++++++++++++++++++ 2 files changed, 203 insertions(+), 8 deletions(-) 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 2bfeebe49..89071cb26 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 @@ -28,9 +28,11 @@ import com.netflix.graphql.types.errors.TypedGraphQLError import graphql.execution.DataFetcherExceptionHandler import graphql.execution.DataFetcherExceptionHandlerParameters import graphql.execution.DataFetcherResult +import graphql.execution.ExecutionStepInfo import graphql.execution.ResultPath import graphql.schema.DataFetcher import graphql.schema.DataFetchingEnvironment +import graphql.schema.DataFetchingEnvironmentImpl import graphql.schema.TypeResolver import org.dataloader.Try import org.slf4j.Logger @@ -88,6 +90,7 @@ open class DefaultDgsFederationResolver() : if (!fetcher.second.parameterTypes.any { it.isAssignableFrom(Map::class.java) }) { throw InvalidDgsEntityFetcher("@DgsEntityFetcher ${fetcher.first::class.java.name}.${fetcher.second.name} is invalid. A DgsEntityFetcher must accept an argument of type Map") } + val result = if (fetcher.second.parameterTypes.any { it.isAssignableFrom(DgsDataFetchingEnvironment::class.java) }) { fetcher.second.invoke(fetcher.first, values, DgsDataFetchingEnvironment(env)) @@ -122,21 +125,23 @@ open class DefaultDgsFederationResolver() : ) .errors( trySequence - .filter { tryResult -> tryResult.isFailure } - .map { tryResult -> tryResult.throwable } - .flatMap { e -> + .mapIndexed { index, tryResult -> Pair(index, tryResult) } + .filter { iter -> iter.second.isFailure } + .map { iter -> Pair(iter.first, iter.second.throwable) } + .flatMap { iter: Pair -> // extract exception from known wrapper types val exception = when { - e is InvocationTargetException && e.targetException != null -> e.targetException - e is CompletionException && e.cause != null -> e.cause!! - else -> e + iter.second is InvocationTargetException && (iter.second as InvocationTargetException).targetException != null -> (iter.second as InvocationTargetException).targetException + iter.second is CompletionException && iter.second.cause != null -> iter.second.cause!! + else -> iter.second } // handle the exception (using the custom handler if present) if (dgsExceptionHandler.isPresent) { + val dfeWithErrorPath = createDataFetchingEnvironmentWithPath(env, iter.first) val res = dgsExceptionHandler.get().handleException( DataFetcherExceptionHandlerParameters .newExceptionParameters() - .dataFetchingEnvironment(env) + .dataFetchingEnvironment(dfeWithErrorPath) .exception(exception) .build() ) @@ -145,7 +150,7 @@ open class DefaultDgsFederationResolver() : sequenceOf( TypedGraphQLError.newInternalErrorBuilder() .message("%s: %s", exception::class.java.name, exception.message) - .path(ResultPath.parse("/_entities")) + .path(ResultPath.parse("/_entities,${iter.first}")) .build() ) } @@ -156,6 +161,29 @@ open class DefaultDgsFederationResolver() : } } + open fun createDataFetchingEnvironmentWithPath(env: DataFetchingEnvironment, pathIndex: Int): DgsDataFetchingEnvironment { + val pathWithIndex = env.executionStepInfo.path.segment("$pathIndex") + val executionStepInfoWithPath = ExecutionStepInfo.newExecutionStepInfo(env.executionStepInfo).path(pathWithIndex).build() + return DgsDataFetchingEnvironment( + DataFetchingEnvironmentImpl + .newDataFetchingEnvironment() + .arguments(env.arguments) + .dataLoaderRegistry(env.dataLoaderRegistry) + .graphQLContext(env.graphQlContext) + .root(env.getRoot()) + .graphQLSchema(env.graphQLSchema) + .fragmentsByName(env.fragmentsByName) + .dataLoaderRegistry(env.dataLoaderRegistry) + .locale(env.locale) + .document(env.document) + .operationDefinition(env.operationDefinition) + .variables(env.variables) + .executionId(env.executionId) + .executionStepInfo(executionStepInfoWithPath) + .build() + ) + } + open fun typeMapping(): Map, String> { return emptyMap() } diff --git a/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/DefaultDgsFederationResolverTest.kt b/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/DefaultDgsFederationResolverTest.kt index 45683260b..216a0e5a7 100644 --- a/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/DefaultDgsFederationResolverTest.kt +++ b/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/DefaultDgsFederationResolverTest.kt @@ -18,6 +18,7 @@ package com.netflix.graphql.dgs import com.apollographql.federation.graphqljava._Entity import com.netflix.graphql.dgs.exceptions.DefaultDataFetcherExceptionHandler +import com.netflix.graphql.dgs.exceptions.DgsEntityNotFoundException import com.netflix.graphql.dgs.exceptions.DgsInvalidInputArgumentException import com.netflix.graphql.dgs.federation.DefaultDgsFederationResolver import com.netflix.graphql.dgs.internal.DgsSchemaProvider @@ -299,6 +300,30 @@ class DefaultDgsFederationResolverTest { assertThat(result.get().data).hasSize(1).first().isInstanceOf(Movie::class.java) assertThat(result.get().data.first() as Movie).extracting { it.movieId }.isEqualTo("1") } + + private fun testEntityFetcherWithoutExceptionHandler(movieEntityFetcher: Any) { + every { applicationContextMock.getBeansWithAnnotation(DgsScalar::class.java) } returns emptyMap() + every { applicationContextMock.getBeansWithAnnotation(DgsDirective::class.java) } returns emptyMap() + every { applicationContextMock.getBeansWithAnnotation(DgsComponent::class.java) } returns mapOf("MovieEntityFetcher" to movieEntityFetcher) + + dgsSchemaProvider.schema("""type Query {}""") + + val arguments = mapOf( + _Entity.argumentName to listOf(mapOf("__typename" to "Movie", "movieId" to "1"), mapOf("__typename" to "Movie", "movieId" to "2")) + ) + val dataFetchingEnvironment = constructDFE(arguments) + + val result = + ( + DefaultDgsFederationResolver(entityFetcherRegistry, Optional.of(dgsExceptionHandler)) + .entitiesFetcher() + .get(dataFetchingEnvironment) as CompletableFuture>> + ) + + assertThat(result).isNotNull + assertThat(result.get().data).hasSize(1).first().isInstanceOf(Movie::class.java) + assertThat(result.get().data.first() as Movie).extracting { it.movieId }.isEqualTo("1") + } } @Nested @@ -458,6 +483,146 @@ class DefaultDgsFederationResolverTest { .satisfies(Consumer { assertThat(it).contains("IllegalArgumentException") }) } + @Test + fun `Entity Fetcher throws DgsEntityNotFoundException contains path in error`() { + val movieEntityFetcher = object { + @DgsEntityFetcher(name = "Movie") + fun movieEntityFetcher(values: Map, dfe: DgsDataFetchingEnvironment?): Movie { + if (dfe == null) { + throw RuntimeException() + } + if (values["movieId"] == "2") { + throw DgsEntityNotFoundException("No entity found for movieId 2") + } + return 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("MovieEntityFetcher" to movieEntityFetcher) + + dgsSchemaProvider.schema("""type Query {}""") + + val arguments = mapOf( + _Entity.argumentName to listOf(mapOf("__typename" to "Movie", "movieId" to "1"), mapOf("__typename" to "Movie", "movieId" to "2")) + ) + val dataFetchingEnvironment = constructDFE(arguments) + + val result = + DefaultDgsFederationResolver(entityFetcherRegistry, Optional.of(dgsExceptionHandler)) + .entitiesFetcher().get(dataFetchingEnvironment) as CompletableFuture>> + + assertThat(result).isNotNull + assertThat(result.get().data).hasSize(2) + assertThat(result.get().errors).hasSize(1).first().extracting { it.path } + .satisfies(Consumer { assertThat(it.toString()).contains("_entities, 1") }) + } + + @Test + fun `Entity Fetcher throws DgsEntityNotFoundException for different types`() { + // Define a mock movie entity fetcher that throws an EntityNotFoundException for movieId 1 + val movieEntityFetcher = object { + @DgsEntityFetcher(name = "Movie") + fun movieEntityFetcher(values: Map, dfe: DgsDataFetchingEnvironment?): Movie { + if (values["movieId"] == "1") { + throw DgsEntityNotFoundException("No entity found for movieId 1") + } + return Movie(values["movieId"].toString(), "Some Movie Title") + } + } + + // Define a mock show entity fetcher that throws an EntityNotFoundException for showId 2 + val showEntityFetcher = object { + @DgsEntityFetcher(name = "Show") + fun showEntityFetcher(values: Map, dfe: DgsDataFetchingEnvironment?): Show { + if (values["showId"] == "2") { + throw DgsEntityNotFoundException("No entity found for showId 2") + } + return Show(values["showId"].toString(), "Some Show Title") + } + } + + // Mock the ApplicationContext to return the mock entity fetchers + every { applicationContextMock.getBeansWithAnnotation(DgsScalar::class.java) } returns emptyMap() + every { applicationContextMock.getBeansWithAnnotation(DgsDirective::class.java) } returns emptyMap() + every { applicationContextMock.getBeansWithAnnotation(DgsComponent::class.java) } returns mapOf( + "MovieEntityFetcher" to movieEntityFetcher, + "ShowEntityFetcher" to showEntityFetcher + ) + + // Initialize the schema with a minimal query type + dgsSchemaProvider.schema("""type Query {}""") + + // Construct arguments with _entities argument + val arguments = mapOf( + _Entity.argumentName to listOf(mapOf("__typename" to "Movie", "movieId" to "1"), mapOf("__typename" to "Show", "showId" to "2")) + ) + + val dataFetchingEnvironment = constructDFE(arguments) + + // Invoke the entitiesFetcher to get the result + val result = DefaultDgsFederationResolver(entityFetcherRegistry, Optional.of(dgsExceptionHandler)) + .entitiesFetcher().get(dataFetchingEnvironment) as CompletableFuture>> + + // Assertions to check the result and errors + assertThat(result).isNotNull + assertThat(result.get().data).hasSize(2) + assertThat(result.get().errors).hasSize(2) + assertThat(result.get().errors[0].path.toString()).contains("_entities, 0") + assertThat(result.get().errors[0].message).contains("No entity found for movieId 1") + assertThat(result.get().errors[1].path.toString()).contains("_entities, 1") + assertThat(result.get().errors[1].message).contains("No entity found for showId 2") + } + + @Test + fun `DgsEntityNotFoundException contains path indexes when multiple entities of same type not found in query`() { + val movieEntityId1 = "111111" + val movieEntityId2 = "222222" + val movieEntityId3 = "333333" + + val movieEntityFetcher = object { + @DgsEntityFetcher(name = "Movie") + fun movieEntityFetcher(values: Map, dfe: DgsDataFetchingEnvironment?): Movie { + if (dfe == null) { + throw RuntimeException() + } + if (values["movieId"] == movieEntityId1) { + throw DgsEntityNotFoundException("No entity found for movieId $movieEntityId1") + } + if (values["movieId"] == movieEntityId2) { + throw DgsEntityNotFoundException("No entity found for movieId $movieEntityId2") + } + return 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("MovieEntityFetcher" to movieEntityFetcher) + + dgsSchemaProvider.schema("""type Query {}""") + + val arguments = mapOf( + _Entity.argumentName to listOf( + mapOf("__typename" to "Movie", "movieId" to movieEntityId1), + mapOf("__typename" to "Movie", "movieId" to movieEntityId2), + mapOf("__typename" to "Movie", "movieId" to movieEntityId3) + ) + ) + val dataFetchingEnvironment = constructDFE(arguments) + + val result = + DefaultDgsFederationResolver(entityFetcherRegistry, Optional.empty()) + .entitiesFetcher().get(dataFetchingEnvironment) as CompletableFuture>> + + assertThat(result).isNotNull + assertThat(result.get().data).hasSize(3).last().isNotNull.hasFieldOrPropertyWithValue("movieId", movieEntityId3) + assertThat(result.get().errors).hasSize(2).satisfiesExactly( + { error -> assertThat(error.path.contains("_entities, 0")) }, + { error -> assertThat(error.path.contains("_entities, 1")) } + ) + } + @Test fun `Invoking an Entity Fetcher missing an argument`() { val arguments = mapOf(_Entity.argumentName to listOf(mapOf("__typename" to "Movie"))) @@ -508,4 +673,6 @@ class DefaultDgsFederationResolverTest { } data class Movie(val movieId: String = "", val title: String = "") + + data class Show(val showId: String = "", val title: String = "") } From f4d5984bd74115d11558159d0eef4a10f181e569 Mon Sep 17 00:00:00 2001 From: mendezm Date: Mon, 25 Sep 2023 15:18:44 -0700 Subject: [PATCH 2/2] Fetch underlying DFE in DgsDataFetchingEnvironment to create new DFE using DataFetchingEnvironmentImpl. --- .../graphql/dgs/DgsDataFetchingEnvironment.kt | 3 ++ .../DefaultDgsFederationResolver.kt | 20 +------- .../dgs/DefaultDgsFederationResolverTest.kt | 50 ++++++++++++++++--- 3 files changed, 48 insertions(+), 25 deletions(-) diff --git a/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/DgsDataFetchingEnvironment.kt b/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/DgsDataFetchingEnvironment.kt index 4451d0e1a..9907cadc1 100644 --- a/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/DgsDataFetchingEnvironment.kt +++ b/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/DgsDataFetchingEnvironment.kt @@ -24,6 +24,9 @@ import graphql.schema.DataFetchingEnvironment import org.dataloader.DataLoader class DgsDataFetchingEnvironment(private val dfe: DataFetchingEnvironment) : DataFetchingEnvironment by dfe { + fun getDfe(): DataFetchingEnvironment { + return this.dfe + } fun getDgsContext(): DgsContext { return DgsContext.from(this) 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 89071cb26..8e1e4cb3d 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 @@ -164,24 +164,8 @@ open class DefaultDgsFederationResolver() : open fun createDataFetchingEnvironmentWithPath(env: DataFetchingEnvironment, pathIndex: Int): DgsDataFetchingEnvironment { val pathWithIndex = env.executionStepInfo.path.segment("$pathIndex") val executionStepInfoWithPath = ExecutionStepInfo.newExecutionStepInfo(env.executionStepInfo).path(pathWithIndex).build() - return DgsDataFetchingEnvironment( - DataFetchingEnvironmentImpl - .newDataFetchingEnvironment() - .arguments(env.arguments) - .dataLoaderRegistry(env.dataLoaderRegistry) - .graphQLContext(env.graphQlContext) - .root(env.getRoot()) - .graphQLSchema(env.graphQLSchema) - .fragmentsByName(env.fragmentsByName) - .dataLoaderRegistry(env.dataLoaderRegistry) - .locale(env.locale) - .document(env.document) - .operationDefinition(env.operationDefinition) - .variables(env.variables) - .executionId(env.executionId) - .executionStepInfo(executionStepInfoWithPath) - .build() - ) + val dfe = if (env is DgsDataFetchingEnvironment) env.getDfe() else env + return DgsDataFetchingEnvironment(DataFetchingEnvironmentImpl.newDataFetchingEnvironment(dfe).executionStepInfo(executionStepInfoWithPath).build()) } open fun typeMapping(): Map, String> { diff --git a/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/DefaultDgsFederationResolverTest.kt b/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/DefaultDgsFederationResolverTest.kt index 216a0e5a7..84087d3f9 100644 --- a/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/DefaultDgsFederationResolverTest.kt +++ b/graphql-dgs/src/test/kotlin/com/netflix/graphql/dgs/DefaultDgsFederationResolverTest.kt @@ -24,13 +24,18 @@ import com.netflix.graphql.dgs.federation.DefaultDgsFederationResolver import com.netflix.graphql.dgs.internal.DgsSchemaProvider import com.netflix.graphql.dgs.internal.EntityFetcherRegistry import com.netflix.graphql.dgs.internal.method.MethodDataFetcherFactory +import com.netflix.graphql.types.errors.ErrorDetail +import com.netflix.graphql.types.errors.TypedGraphQLError +import graphql.GraphQLError import graphql.execution.DataFetcherExceptionHandler import graphql.execution.DataFetcherExceptionHandlerParameters import graphql.execution.DataFetcherExceptionHandlerResult import graphql.execution.DataFetcherResult import graphql.execution.ExecutionStepInfo +import graphql.execution.MergedField import graphql.execution.ResultPath import graphql.execution.TypeResolutionParameters +import graphql.language.Field import graphql.schema.DataFetchingEnvironment import graphql.schema.DataFetchingEnvironmentImpl import graphql.schema.GraphQLList @@ -519,7 +524,7 @@ class DefaultDgsFederationResolverTest { } @Test - fun `Entity Fetcher throws DgsEntityNotFoundException for different types`() { + fun `Entity Fetcher throws DgsEntityNotFoundException for different types with custom handling`() { // Define a mock movie entity fetcher that throws an EntityNotFoundException for movieId 1 val movieEntityFetcher = object { @DgsEntityFetcher(name = "Movie") @@ -560,18 +565,48 @@ class DefaultDgsFederationResolverTest { val dataFetchingEnvironment = constructDFE(arguments) + // Create a custom exception handler which uses fields in DFE available when doing custom handling. + val customExceptionHandler = object : DataFetcherExceptionHandler { + override fun handleException(handlerParameters: DataFetcherExceptionHandlerParameters?): CompletableFuture { + return if (handlerParameters?.exception is DgsEntityNotFoundException) { + // Check DFE field + val fieldName = handlerParameters.dataFetchingEnvironment.field.name + + val exception = handlerParameters.exception + val graphqlError: GraphQLError = + TypedGraphQLError + .newBuilder() + .errorDetail(ErrorDetail.Common.ENHANCE_YOUR_CALM) + .message("$fieldName Error: " + exception.message) + .path(handlerParameters.path) + .build() + CompletableFuture.completedFuture( + DataFetcherExceptionHandlerResult.newResult() + .error(graphqlError) + .build() + ) + } else { + dgsExceptionHandler.handleException(handlerParameters) + } + } + } + // Invoke the entitiesFetcher to get the result - val result = DefaultDgsFederationResolver(entityFetcherRegistry, Optional.of(dgsExceptionHandler)) + val result = DefaultDgsFederationResolver(entityFetcherRegistry, Optional.of(customExceptionHandler)) .entitiesFetcher().get(dataFetchingEnvironment) as CompletableFuture>> // Assertions to check the result and errors assertThat(result).isNotNull assertThat(result.get().data).hasSize(2) - assertThat(result.get().errors).hasSize(2) - assertThat(result.get().errors[0].path.toString()).contains("_entities, 0") - assertThat(result.get().errors[0].message).contains("No entity found for movieId 1") - assertThat(result.get().errors[1].path.toString()).contains("_entities, 1") - assertThat(result.get().errors[1].message).contains("No entity found for showId 2") + assertThat(result.get().errors).hasSize(2).satisfiesExactly( + { error -> assertThat(error.path.toString().contains("_entities, 0")) }, + { error -> assertThat(error.path.toString().contains("_entities, 1")) } + ) + assertThat(result.get().errors).hasSize(2).satisfiesExactly( + { error -> assertThat(error.message.contains("No entity found for movieId 1")) }, + { error -> assertThat(error.message.contains("No entity found for movieId 2")) } + ) + assertThat(result.get().errors[0].message).contains(dataFetchingEnvironment.getDfe().field.name) } @Test @@ -668,6 +703,7 @@ class DefaultDgsFederationResolverTest { .newDataFetchingEnvironment() .arguments(arguments) .executionStepInfo(executionStepInfo) + .mergedField(MergedField.newMergedField(Field("Movie")).build()) .build() ) }