Plugin didEncounterErrors should allow a way to add additional information from context

[Sytten]

As per other issues, the current recommended way to access the context when handling error in a plugin is to use the hook didEncounterErrors. Something similar to:

export const GraphQLErrorsHandler: ApolloServerPlugin<Context> = {
  requestDidStart(a) {
    return {
      didEncounterErrors(reqCtx) {
        const errors = reqCtx.errors || []
        for (const error of errors) {
          handleError(error, reqCtx.context)
        }
      },
    }
  },
}

The problem is that those error are all readonly and cannot really be modified. For example, it is not possible to add extensions if the caller did not define it as not null. Thus there is not reliable way to add information to the error that could be used by formatError down the line.

A very common use case is adding a request ID to the extensions in case of an error so we can match the backend logs with the frontend error. My current work around is to add the information to the originalError:

  const cause = error.originalError as ExtendedError
  cause.requestId = ctx.request.id

And then read it back in the formatError to add it to the extensions. But I realized recently that originalError is not always set, so my workaround fails sometimes. We also have to note that properties attached to the GraphQLError sent to didEncounterErrors are not carried over in formatErrors for some reason.

For all those reasons, I think there should be an official way to deal with that problem since it was possible to do it with Extensions that are now deprecated.

[Sytten]

Relates to #631, #1343 and #2355

[ch1ll0ut1]

Another Problem is: The error object in formatError contains additional data of the original error. This is completely missing in all ApolloServer plugin hooks. The error object there is already a stripped and formatted version by ApolloServer and so we cant use that for logging details of e.g. database errors.

We simply need: formatError with context... this will solve all problems

My current use case is: I want to use Sentry to log errors in Graphql resolvers. I want to put all the query details into the Sentry meta informations. This are only available during formatError, however i need the context to access request information to have them as well in the Sentry meta informations.

So there is just no way around either fixing the Plugin hooks to expose the original error or adding context to formatError.

[amatiushkin]

There are use cases to propagate information with error, which is request-specific. This would be nice to have.

Think about request traceability between multiple components, Apollo GW -> Apollo Server. HTTP header have transaction unique identifier (myproduct-transaction: <random string>). It will help if this transaction id could be added to error in one place using formatError or some other methods out of the box instead of handling it within application code.

[fuads96]

I just write the plugin and it works properly:

Edited: Warning!!!! Don't use the code below, I just keep it for documention purposes. see the new answer

module.exports = {
    requestDidStart: (reqCtx) => {
        return {
            didEncounterErrors: ({ errors, request, context }) => {
                errors.forEach(({ extensions }) => {
                    extensions.request = request;
                    extensions.context = context;
                })
            },
        }
    },
}

Note: delete context in formatError to avoid sending back to users:

formatError.js

const handler = (error) => {
      const context = error.extensions.context;
      // do something with context...

      delete error.extensions["context"];

      return error;
} 

[Sytten]

extensions is not always defined, so this method is not 100% reliable. I don't remember exactly when it was not defined in my tests.

[fuads96]

extensions is not always defined, so this method is not 100% reliable. I don't remember exactly when it was not defined in my tests.

Unfortunately Yes! extensions isn't writable and when it's undefined or null we can't change it to object and then adding extra props. But I have another idea:

we can use the message prop (which is writable) as a transport layer, so I changed my code as shown below:

module.exports = {
    requestDidStart: (reqCtx) => {
        return {
            didEncounterErrors: ({ errors, request, context }) => {
                errors.forEach(error => {
                    const msg = error.message;
                    error.message = {
                        request,
                        context,
                        toString: () => msg,
                    }
                })
            },
        }
    },
}

Then we can access additional info in formatError and change message to the default string format (for serializing purposes):

const handler = (error) => {
      const { message, locations, path, extensions = {} } = error;
      const { request, context } = message;

      error.message = message + "";

      return error;
} 

[glasser]

This seems valuable but I'm going to remove it from AS3 blockers because I don't think fixing this needs to be a breaking change: we can start passing context as a second argument to formatError callbacks or allow RequestContext.errors to be mutated without breaking any existing code.

It's tempting to just drop the readonly on RequestContext.errors or change it to be an array rather than ReadonlyArray, but this presents some trickiness — eg, if you delete all the errors, are we still sending an error response? I realize that @abernix recently commented that we're trying to keep the formatError API similar to the graphql-js version of it and that didEncounterErrors would be a better place to improve things, but I'm leaning towards passing context to formatError as a better approach since that lets us keep the constraint that the number of errors that exists before didEncounterError matches the number of errors afterwards.

[Sytten]

The alternative is to allow the modification of extensions in the plugin. Which would be a good thing anyway IMO.

[glasser]

The challenge is that extensions are readonly on the core GraphQLError, not something that's part of apollo-server...

We could I suppose provide a mechanism for letting you replace specific errors in the list. Or just document it as "if you delete all the errors, you get to keep both pieces".

[RichardWright]

Can I just do a plus one for adding the context to the error formater? I'm trying to get operationName and the variables on the error but this is very difficult currently.

[glasser]

@IvanGoncharov I wonder if you have any thoughts on this? I know you've been thinking about how to simplify apollo-server-errors.

[RichardWright]

another question on this - why is the error type on didencountererrors and formatError 'GraphqlError' and not 'ApolloError'? Thrown apollo errors lose their codes and make defining them totally pointless?

[alexkrolick]

Would it be possible to have some/all plugins run after formatError? We are having the same issue of needing to log errors, but we do classification and tagging of the errors using information that is only available in the extensions field available in the formatError lifecycle.

We considered moving a "formatError" plugin before the other plugins, but the errors available to plugings are readonly and sometimes lack the extensions or originalError fields.

[IvanGoncharov]

@IvanGoncharov I wonder if you have any thoughts on this? I know you've been thinking about how to simplify apollo-server-errors.

@glasser Sorry, lost it in my GH inbox.
Just thoughts in random order:

• All error fields should be readonly.
• Context should be part of step in pipeline that is not shared between client and server. For example, both client and server should parse queries in exactly the same way so parsing step shouldn't have context attached. Since error handling is done purely on a server that mean it should have access to context.
• In graphql@16.0.0 I made extensions to be always present (no more null or undefined). This is to make reading extensions easier, not to encourage people to mutate it.
• graphql-js version of formatError was a simple serialization mechanism and wasn't design as signature for "pipeline callback" as it used in AS. Moreover I deprecated it in graphql@16.0.0 so now it's just a wrapper around GraphQLError.toJSON.

In general: I think we need to overhaul error handling both in AS and graphql-js

[alexkrolick]

How are you supposed to decorate an error object in a plugin if they are readonly?

+1 to overhauling error handling. This area feels very circular. You can transform in formatError but can't access the request context for logging/tracing, and in plugins you can do so but get the error before it is transformed and can't pipeline the data you need by adding fields 🔁.

[simlu]

Please add a context to formatError... The hours I have wasted because I can't figure out how to do a thing that should be simple 😢

[glasser]

@simlu Part of the challenge here is that formatError can be called both from within the "request pipeline" once the context is set up... or earlier because there's a problem recognizing the request at all. So it's challenging to consistently provide useful context to this function. But perhaps we could at least add the GraphQLRequest context as a parameter to the function when it is invoked from the request pipeline, and leave it null when invoked from "parsing the request"? @IvanGoncharov thoughts?

[Jomik]

We would also like a way to add to extensions during an error handling callback.
Envelop supports this by asking the plugin to return a new list errors: https://www.envelop.dev/plugins/use-sentry
Could Apollo allow similar, we definitely need this to link errors happening in the backend, to the on our users report in.

[glasser]

I put some effort into making modifying the errors array (as opposed to just mutating the errors in it) OK in #6927. But I reverted it in #6931. Allowing you to remove all the errors from the array led to weird cases when, eg, it was a parse or validation error: suddenly the request pipeline is trying to issue a parse error that has no errors?

A valid option here would be to do the handling in willSendResponse rather than didEncounterErrors. By this point the errors have moved from requestContext.errors to requestContext.response.body (and have already been "formatted") and you can do arbitrary changes to it without it messing up the logic of the request pipeline.