Callback returned from executionDidStart is not triggered when error occurs

[jsarafajr]

I'm trying to implement a plugin that should receive an error when query execution fails. I have the following plugin code:

const plugin = {
  requestDidStart: () => {
    return {
      executionDidStart: () => {
        return (err) => {
          if (err) {
            console.log('never called');
          }
        };
      },

      didEncounterErrors: (err) => {
        console.log('this one is called');
      }
  };
}

When I throw an error in a resolver didEncounterErrors gets called but callback returned from executionDidStart does not.

After a quick debug I found that error is not re-thrown when result returned from execution has errors property: https://github.com/apollographql/apollo-server/blob/master/packages/apollo-server-core/src/requestPipeline.ts#L370

My assumption is that instead of calling didEncounterErrors it should probably call emitErrorAndThrow.

[brianjquinn]

you beat me to it. I was seeing slightly different behavior, however. When I throw an error in a resolver I was seeingdidEncounterErrors being called here: https://github.com/apollographql/apollo-server/blob/master/packages/apollo-server-core/src/requestPipeline.ts#L370 and then the executionDidStart callback (executionDidEnd) being called after without an error object being passed in: https://github.com/apollographql/apollo-server/blob/master/packages/apollo-server-core/src/requestPipeline.ts#L378.

In either case you would expect the *DidEnd() callback to be called first with an error object passed in and then the didEnounterErrors to be called after (just like it does with the parse and validation phases).

When I throw an error in a resolver didEncounterErrors gets called but callback returned from executionDidStart does not.

I think to correct that (^) statement you mean the callback is called but err is always undefined so you never enter that if block (which mirrors the behavior I described).

[glasser]

I think this is working as intended. Passing a single error to executionDidEnd doesn't make sense: execution can produce lots of errors, for various fields. You can look at requestContext.errors to see if there are errors.