apollographql / apollo-server

🌍  Spec-compliant and production ready JavaScript GraphQL server that lets you develop in a schema-first way. Built for Express, Connect, Hapi, Koa, and more.
https://www.apollographql.com/docs/apollo-server/
MIT License
13.76k stars 2.03k forks source link

Logic error on resolvers' error handling #374

Closed nuxlli closed 7 years ago

nuxlli commented 7 years ago

I'm trying to raise an exception that returns a http status code 401:

Query: {
  currentUser(_root, _args, { request }) {
    if (!request.auth) {
      const error = new Error('You must be authenticated');
      error.statusCode = 401;
      throw error;
    }
    // ...
  }
}

But there is a problem with errors handling logic (took hapi as example but the error is consistent with other packages).

In https://github.com/apollographql/graphql-server/blob/master/packages/graphql-server-hapi/src/hapiApollo.ts#L28 would be expected that the runHttpQuery returns a rejected promise with the error being raised within the resolver.

However if we observe graphql.execute's code it's possible to note that it never returns errors generated within the resolver: https://github.com/graphql/graphql-js/blob/master/src/execution/execute.js#L157

I think in fact this is a bug (or a mistaken implementation) of graphql lib but it directly affects the lib anyway.

I'm new to the graphql's community and I'd be really greatful if you could shed a light on how do I proceed with this problem:

helfer commented 7 years ago

@nuxlli As you pointed out, this is perfectly expected behavior in graphql-js. Errors in resolve functions are captured and returned as part of the response in the {errors} field.

The general recommendation for what you're attempting to do is to have an authentication middleware before graphql even gets executed. If the user is not authenticated, that middleware will raise an error, return a 401, and the graphql query won't even be executed.

I'm going to close this. If this is of concern for you, I think graphql-js would be the right repo to discuss the issue.

ak99372 commented 6 years ago

There are situations when you might need to throw a non-application error (e.g. related to server or infrastructure, DB connection.. etc) from within resolver or its child function.

runHttpQuery should handle [GraphQLErrors] but it should not swallow all Errors by default.