500 Error handling suggestions

[dburles]

The way 500 errors are currently handled is that exceptions are caught and GraphQL sends the literal error message back to the consumer in the error response, such as "message": "foo is not a function" and the error trace is swallowed. Instead, it would be preferable if it were a generic message, such as Internal Server Error, and the error were logged.

https://github.com/contrawork/graphql-helix/blob/12895db29a081afc710ab6532937e7129bd0bd8b/packages/core/lib/process-request.ts#L246-L248

[n1ru4l]

@dburles We could introduce a safe by default error handler function that can be overwritten. The default implementation could simply map the error message to Internal server error and print the original error to the console with console.error? If people want to customize behavior they can override it. @dotansimha What do you think?

[dburles]

That sounds like an ideal solution to me!

[dotansimha]

@dburles We could introduce a safe by default error handler function that can be overwritten. The default implementation could simply map the error message to Internal server error and print the original error to the console with console.error? If people want to customize behavior they can override it. @dotansimha What do you think?

Agree! Sounds good :)

[dburles]

There will also need to be a way to expose errors that are explicitly thrown, such as argument validation messages etc.

[n1ru4l]

Those should not be thrown but instead the execute/validate functions should return the corresponding errors, or what are you referring to?

[dburles]

I think a good solution might be to check for the presence of an expose property on the error object. Also, status would be great too, so users can set a custom http status if they wish.

// In processing the request:
if (error.expose) {
  // return error.status and error message
} else {
  console.error(error);
  // return 500 status and Internal Server Error message
}

This way it's simple for users to create their own error handlers that allow the server to expose their messages. e.g.

class ValidationError extends Error {
  constructor(message) {
    super(message);
    this.expose = true;
  }
}

Added bonus, a Sentry integration can very easily skip sending these errors:

  beforeSend(event, hint) {
    const error = hint.originalException;
    if (error && error.expose) {
      return null;
    }
    return event;
  },

[dburles]

Those should not be thrown but instead the execute/validate functions should return the corresponding errors, or what are you referring to?

I mean just validation like:

  resolve(obj, { id }) {
    if (!id) {
      throw Error("Argument ‘id’ is invalid.");
    }

[n1ru4l]

Those should not be thrown but instead the execute/validate functions should return the corresponding errors, or what are you referring to?

I mean just validation like:

  resolve(obj, { id }) {
    if (!id) {
      throw Error("Argument ‘id’ is invalid.");
    }

This will never result in the execute function to throw. It will be included in the errors array returned from execute. If you want to mask your schema resolver errors we recommend using envelop and the masked errors plugin https://www.envelop.dev/plugins/use-masked-errors

[n1ru4l]

If you want to customize the status code you can add extensions to the graphql errors thrown in the resolvers and then loop through those errors and set the correct status code before sending the result. This is not something that should be baked into helix as it shall remain unopinionated.

[dburles]

Right, I think I see what you're saying. These errors won't be affected by your proposed changes and they can be handled independently.

[n1ru4l]

I think helix should only handle unexpected errors. E.g. if the schema is invalid or undefined etc or execute throws an error for some unknown reason

[dburles]

Hey @n1ru4l do you have much of an idea how you might want this to function? I could take a look at it.

[n1ru4l]

Also not sure whether using a status code makes sense at all in terms of GraphQL. Especially when using stuff like defer and stream, the error can occur after the status code must be determined 🤔