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: https://github.com/Netflix/dgs-framework/pull/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 https://github.com/Netflix/dgs-framework/blob/f9382bff5c8a5f9af70087c160c7f82d632c1b1f/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/GraphQLJavaErrorInstrumentation.kt#L53-L57

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: https://github.com/Netflix/dgs-framework/blob/f9382bff5c8a5f9af70087c160c7f82d632c1b1f/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/GraphQLJavaErrorInstrumentation.kt#L56-L57

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. https://github.com/Netflix/dgs-framework/blob/8dd6966b216f77b0d29d4654094cd181330bc752/graphql-error-types/src/main/java/com/netflix/graphql/types/errors/ErrorDetail.java#L151

Is this the expected behaviour for all DataFetchingException?

[djlfb]

This is also causing problems for us, we have our own error shape. Setting GraphQLError::getErrorType to return null will allow us to bypass the instrumentation but this isn't ideal.

Could there be an option to make the typed error mapping configurable?

[srinivasankavitha]

@mrvaruntandon - you are right in that setting the ErrorDetail to SERVICE_ERROR, the INTERNAL error type is overridden and always set as UNAVAILABLE. This can be easily fixed by adding a new code for the ErrorDetail - perhaps one for describing Data fetching exceptions specifically.

@djlfb - The framework generally is expected to convert all errors to a TypedGraphQLError. The GraphQLJavaErrorInstrumentation class handles some cases that were previously not handled and hence were exposing the graphql-java error as is. For example, we also have our default exception handler that does the same - https://github.com/Netflix/dgs-framework/blob/8dd6966b216f77b0d29d4654094cd181330bc752/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/exceptions/DefaultDataFetcherExceptionHandler.kt Shouldn't that also cause issues for you?

It is easy enough to make GraphQlJavaErrorInstrumentation configurable, but that wouldn't entirely solve your issue unless you are also disabling the behavior in the default exception handling.

Lastly, to address the original question in the issue from @hpuac - it is intentional to override the original error type and set it to INTERNAL in the case of data fetching exceptions. Is that not descriptive enough? We could also consider introducing a new ErrorDetail code, e.g. DATAFETCHER_ERROR.

[djlfb]

@srinivasankavitha I appreciate the rationale, but for us it isn't ideal. We have our own DataFetcherExceptionHandler implementation and don't use DefaultDataFetcherExceptionHandler as a result of our requirements.

By setting a new ErrorClassification we can bypass this implementation, but it'd still be preferable to not have that instrumentation in place for us.

[srinivasankavitha]

For your use case, we could make this particular instrumentation configurable so you can disable it. @paulbakker - thoughts?

[paulbakker]

Fixed in #2124