Apollo-server: `didEncounterErrors` is not triggered when an error occurs in `context` creation.

[alexstrat]

Bug

@abernix edits: The didEncounterErrors life-cycle hook is not invoked when errors occur within context creation. That shouldn't be the case!

Original feature request

As of version 2.9.0, there is no lifecycle event to handle manually errors encountered in the context creation process.

Suggestion:

const server = new ApolloServer({
  context: () => { throw new Error('Ouch!') },
  plugins: [
    {
      requestDidStart: () => ({
        contextCreationDidFail: (error) => {
          // do something here?
        },
      })
    },
  ],
})

[abernix]

This is a severe error if it happens, which is why we wrap it for you and throw a pre-defined error.

If you have error-prone code in your context function, you should guard it with a try / catch. This isn't something that should be handled at the plugin level, though, the actual error message itself should (hopefully, though I haven't checked) propagate to the didEncounterErrors hook.

[abernix]

Out of curiosity though, what would you propose happen in contextCreationDidFail? (i.e. your do something here?)

[alexstrat]

Specifically, didEncounterErrors is not triggered when an error occurred in context creation process. Like you, I expected it was, that's why I opened this issue in the first place.

Though, I thought didEncounterErrors was not the ideal event for this kind of situation because the current signature (didEncounterErrors(requestContext)) implies that it happens after the context was successfully created. That's why I suggested a contextCreationDidFail(error).

I use the didEncounterErrors to report errors to an error monitoring and reporting tool. That's what I would do in the contextCreationDidFail as well.

Wrapping context function in a try/catch does work, of course. Though one can consider context creation as part of the request lifecycle, and having a hook for this kind of situation can fall into the range of features of the Appollo plugin API. Hence the feature request.

[abernix]

The naming is a bit confusing but requestContext is representative of Apollo Server (the framework's) context, not the user's context. Put another way, the requestContext has a property on it called context, which is the actual context that the end-user creates within the context function.

To me, it sounds like a bug that didEnounterErrors is not invoked when there is a user-context creation error, and it seems that we should consider addressing that. Therefore, I've adjusted this feature request to an issue, accordingly.

[maximgeerinck]

Having the same problem, we throw an UnauthorizedException when we determined in the context if the user is not authorized. this is not caught in the didEncounterErrors plugin function.

[stephenh]

We also had this issue, we had a production incident with errors being created by from context blowing up, but there was zero console output from the server.

Neither the logger nor the plugin are hit if context throws:

    plugins: [errorLogging],
    logger: {
      error: (msg: any) => console.log(msg),
      warn: (msg: any) => console.log(msg),
      debug: (msg: any) => console.log(msg),
      info: (msg: any) => console.log(msg),
    },

...

const errorLogging: PluginDefinition = {
  requestDidStart() {
    return {
      didEncounterErrors(ctx) {
        const { errors, context } = ctx;
        if (errors) {
          errors.forEach((err) => {
            context.logger.error({ err });
          });
        }
      },
    };
  },
};

That said, I get that didEncounterErrors wants to use our context which doesn't exist, hence the contextCreationDidFail suggestion, which I'm +1.

The only way we were able to see a glimpse of the error, was in our user's response payloads in the chrome dev console Network tab:

{"errors":[{"message":"Context creation failed: foo","extensions":{"code":"INTERNAL_SERVER_ERROR","exception":{"stacktrace":["Error: Context creation failed: foo","    at ApolloServer.context (/home/node/app/src/server.ts:60:13)","    at ApolloServer.<anonymous> (/home/node/app/node_modules/apollo-server-express/node_modules/apollo-server-core/src/ApolloServer.ts:847:24)","    at Generator.next (<anonymous>)","    at fulfilled (/home/node/app/node_modules/apollo-server-express/node_modules/apollo-server-core/dist/ApolloServer.js:5:58)","    at processTicksAndRejections (internal/process/task_queues.js:97:5)"]}}}]}

(Using the apollo-server-express 2.16.1.)

I'm fairly ambivalent about how this is handled, but even fwiw Apollo directly doing a hard-coded console.log seems preferable stopgap to this being silently dropped (from a console/logging perspective), so just a +1 / bump.

[johnculviner]

This is definitely unexpected behavior for me - glad I had this happen before I went to production as zero logging is not good when you think you have your bases covered with requestDidStart.didEncounterErrors.

I wouldn't be surprised if half the people using apollo have something like this in their code (that ends up being a gotcha).

context: ({ req }) => {
    const oAuthToken = req.headers.authorization
    if (!oAuthToken) throw new AuthenticationError('OAuth Token Missing')

Maybe this could be added to the docs prominently if you want to not have this go to requestDidStart.didEncounterErrors where I think many would expect it.

Or maybe rename to requestDidStart.sometimesWhenDidEncounterErrors? 😄

[spencerwilson]

Want to offer a use case:

If you have error-prone code in your context function

We do! In fact, it was inspired by the guidance in "API-wide authentication". Our GraphQL API serves only confidential data and privileged mutations, so denying all requests that lack valid authentication is appealing to us.