bug: BaseDgsQueryExecutor silently handles exceptions from instrumentations

[Lajcik]

The BaseDgsQueryExecutor silently handles runtime exceptions. I've narrowed it down to the following else block: https://github.com/Netflix/dgs-framework/blob/8fb490c5b4f0aa012c998dd2855abc2301af6ee4/graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/BaseDgsQueryExecutor.kt#L141-L151

This comes up when you start implementing your own instrumentations and make any sort of mistakes.

Expected behavior

Exception details would be printed in the logs (preferably on the error level) in addition to current handling

Actual behavior

Exception is silently handled (transformed into a 500 error)

Steps to reproduce

Create an instrumentation that throws an exception, similar to:

public class ExceptionProducingInstrumentation extends SimplePerformantInstrumentation {

    public DataFetcher<?> instrumentDataFetcher(DataFetcher<?> dataFetcher,
                                                InstrumentationFieldFetchParameters parameters,
                                                InstrumentationState state) {
        if (true) 
            throw new NullPointerException("I made an oopsie");
        return super.instrumentDataFetcher(dataFetcher, parameters, state);
    }
}

[srinivasankavitha]

Hi - actually you need to wrap the throwing of the exception in a DataFetcher and return that. This will allow the default exception handler to trigger and do the needful. You can also add your own custom exception handler if you want more control over the result.

public @NotNull DataFetcher<?> instrumentDataFetcher(DataFetcher<?> dataFetcher, InstrumentationFieldFetchParameters parameters, InstrumentationState state) {
        if (true) {
            return DataFetcher -> {
                throw new NullPointerException("I made an oopsie");
            };

        }
        return super.instrumentDataFetcher(dataFetcher, parameters, state);
    }

Will result in the following:

  "errors": [
    {
      "message": "java.lang.NullPointerException: I made an oopsie",
      "locations": [
        {
          "line": 3,
          "column": 1
        }
      ],
      "path": [
        "hello"
      ],
      "extensions": {
        "errorType": "INTERNAL"
      }
    }
  ],
  "data": {
    "hello": null
  }
}

[Lajcik]

That's a good tip, thanks. Still, I would argue the framework shouldn't swallow exceptions like that.

[srinivasankavitha]

That's a good tip, thanks. Still, I would argue the framework shouldn't swallow exceptions like that.

I see your point. However, we are leveraging graphql-java's execution here, so in order to get all of the properly formatted response with errors as part of the result, we need to go through the exception handling mechanism. You could argue that we can format the same where we catch the exception, but we do not have all information at that point, we just have the message, which I suppose we can add to the error that we construct.

[Lajcik]

I'm not suggesting changing how exceptions are handled, only asking that it is properly logged in this case (ie to add logger.error()) - that would make debugging easier :)

[srinivasankavitha]

I see, yes, we can very well do that.