bug: GraphQLJavaErrorInstrumentation changes error type of DataFetchingException

[hpuac]

Hey folks, I'd like to report a change in behavior that I currently consider a bug. It was introduced in 8.6.0 with this PR: #1905.

Please let me know your opinion on this topic 🙂

Expected behavior

When returning a DataFetcherResult containing a GraphQLError I'm expecting my error to be propagated to the client, including my given code and errorType.

Actual behavior

The GraphQLJavaErrorInstrumentation is creating a new TypedGraphQLError and overriding the errorType.
The code is kept but the errorType is overridden.
See

dgs-framework/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/GraphQLJavaErrorInstrumentation.kt

Lines 53 to 57 in f9382bf

	} else if (error.errorType == graphql.ErrorType.DataFetchingException) {
	val graphqlErrorBuilder = TypedGraphQLError
	.newBuilder()
	.errorType(ErrorType.INTERNAL)
	.errorDetail(ErrorDetail.Common.SERVICE_ERROR)

Steps to reproduce

Return a DataFetcherResult that contains a custom GraphQLError.
Let me know in case you would like to see a demo project.

[kailyak]

Hi @hpuac, thank you for the feedback and report.

To make sure I'm understanding: the use case is, in the case of a customized GraphQLError arising from data fetching, the customized fields (in this case the errorType) should be returned to the client without any modifications? And that this behavior is no longer occurring with the linked change?
If you have a demo project available that would be great to see, otherwise we will investigate further and try and reproduce.

[kailyak]

Hello, I replicated the behavior via this datafetcher, let me know if it's a different case:

@DgsQuery
fun dataFetcherResult(): DataFetcherResult<String> {
  return DataFetcherResult.newResult<String>().error(
    GraphQLError.newError()
    .message("A DataFetchingException error occurred while datafetching").errorType(graphql.ErrorType.DataFetchingException).build()
  ).build()
}

In the linked PR, the goal was to make error reporting more consistent in the framework and it closed a gap that was there previously, which is where your bug report comes in.
We do want to keep the more consistent error handling. In this case, to keep the error info communicated back to the client, two options you could try is adding the ErrorType in the extensions or in the error message.

[mrvaruntandon]

Hi @kailyak,

Referring to the snippet shared in the original issue message:

dgs-framework/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/GraphQLJavaErrorInstrumentation.kt

Lines 56 to 57 in f9382bf

	.errorType(ErrorType.INTERNAL)
	.errorDetail(ErrorDetail.Common.SERVICE_ERROR)

I believe the errorType specified in Line Number 56 is overridden by the errorDetail in the next line (line number 57) i.e. we would never see response with errorType as INTERNAL rather based on the errorDetail specified it would be converted to UNAVAILABLE always.

dgs-framework/graphql-error-types/src/main/java/com/netflix/graphql/types/errors/ErrorDetail.java

Line 151 in 8dd6966

SERVICE_ERROR(UNAVAILABLE),

Is this the expected behaviour for all DataFetchingException?