support for adding optional error handling function before returning …

[tim-steele]

#116 the ability to modify responses and the status codes based on errors.

Gives the option to handle errors from responses before sending back the request. If you do not want to handle the errors, you can omit the optional handleErrors function and it will behave as normal. Since it is optional, it shouldn't break existing functionality.

[ghost]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at [email protected]. Thanks!

[tim-steele]

I signed the CLA.

[tim-steele]

Example

As a quick and dirty example how something like this may work.

My query might look like this:

const productDetails = {
  type: new NonNull( new List( ProductItemType ) ),
  async resolve( root, args, info ) {

    await fetch( url )
      .then( function ( response) {
        if ( response.status != 200 ) {
          throw new httpError( response.status, "Error querying product." );
        };

        return response.json();
      }

  }
};

And my GraphQL Options may look like:

expressGraphQL( function( request ) {
  return {
      schema,
      graphiql: false,
      rootValue: { request: request },
      pretty: true,
      formatError: error => ({
        message: error.message,
        status: error.statusCode
      }),
      handleErrors: (result, response) => {
        if ( result.data.product === null ) {
          response.statusCode = result.errors[ 0 ].status;
        }
      }
    };
  });

[jdosullivan]

+1 to this. I think error handling is the biggest pain for express-graphql. Everything is a 200 OK.

[helfer]

I think something like formatResponse which we have in apollo server would perfectly cover this use-case, and more. I'm all for having functionality like that in express-graphql, but I think it should be done as a generic formatResponse function, not something error specific.

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[tim-steele]

@helfer It wouldn't be hard to change it to formatResponse if the express-graphql team would prefer that over the handleErrors method. I am not sure if having a function that modifies any and all responses aligns with their ideology or not.

Ultimately, my thinking was not allowing you to set a function that modifies any response, but a function that gives you an opportunity to modify the response based on a specific situation: the query has errors.

Ideally, I would want a way to mark certain queries as important or required and move away from a generic function. Using online shopping as an example: I can still display the page to the user and even cache the response if the recommended products query fails, but I want to return a proper error if the product details query fails, because that is critical to the experience.

As I started thinking through it, I realized it isn't that simple and I began to open Pandora's SDK. For example, If I can mark queries as required, can I mark more than 1 query as required? If so, I would then need to start looking at ways at rank queries by importance to overcome certain hurdles: if you have two required queries and they both blow up for different reasons, what HTTP status code do you return or do you throw your hands up in the air and just return something that says "This whole thing didn't work the way you needed it to"? Do you want to treat different HTTP status codes differently? Do you want to display different status codes differently for different queries?

I started going down the rabbit hole trying to find an abstract solution that would fit a lot of different use cases. Ultimately, I decided that the approach in the PR is the simplest and fastest way to address the problem right now, and if this is a feature people really want then it needs more fleshing out and we can have a discussion around the best approach.

[tim-steele]

@helfer my last comment was a little long and may be confusing so I wanted to clarify -- if I change it to formatResponse instead of handleErrors, will the express-graphql team approve the PR?

[helfer]

@tim-steele I was just trying to provide some feedback for what I thought would be a more general solution, but I can't actually speak for the express-graphql team (which I think is just Lee). Having followed other similar issues that have asked for features to be added to express-graphql, I suspect that the chances are rather low, because this isn't core to a reference implementation, and it could theoretically be implemented with an extra middleware.

I think it's pretty reasonable to keep the number of features in the reference implementation pretty small, but there's also a need for a graphql server that has modules for all the bells and whistles you'd want on a production graphql server, which is why I started the apollo-server project with others. My hope is that the features that turn out to be generally useful in apollo-server will eventually make it back into the reference implementation.

[leebyron]

Hey all, sorry for the delay getting to this.

I actually think this is the wrong approach to the original problem.

@helfer I'd be very concerned adding something like formatResponse since it suggests that servers should have the ability to arbitrarily re-shape the response, making it too easy to break from the spec or add other kinds of behavior that make building stable and portable clients more difficult. Perhaps a separate hook should be provided to add the "extensions" key described in http://facebook.github.io/graphql/#sec-Response-Format, but otherwise the spec disallows editing the top level object, so a formatResponse just makes it way too easy to break spec.

Also, there is already a hook called formatError to allow for adding arbitrary metadata to the errors themselves. This is the appropriate hook for altering metadata.

Finally, the original problem suggests bubbling up a status code from an error object to the http level. This goes against GraphQL best practices (http://graphql.org/learn/thinking-in-graphs/) by trying to shoe-horn a query engine into thinking in terms of endpoints. HTTP response codes should only ever model the result of fulfilling the query itself, not any data within the query.

For example, that initial motivating example seems flawed:

      handleErrors: (result, response) => {
        if ( result.data.product === null ) {
          response.statusCode = result.errors[ 0 ].status;
        }
      }

What if the first error doesn't provide a status code but a second one does? What if there are two errors that both have status codes but they're different? Why should the first one win and the second one be ignored? Finally, what if some portion of the query resolved perfectly fine, and only a sub-tree resulted in an error? The behavior above would be incorrect for all three, and so we shouldn't make it easy to add that kind of logic.

However, we already have a mechanism in GraphQL for declaring that data must exist to be legal, and that's the Non-Null type. Top level fields in a query may be Non-Null and if that's the case, then the entire data payload will be null. The GraphQL spec describes this as an "query error" however we're currently responding 200 for this case, which seems like a bug that should be fixed. I'll work on doing exactly that!

[tim-steele]

@leebyron Hey Lee, thanks for responding.

I think the fact that the null query was returning 200 sent me down this path and fixing could solve the original problem. We had the top level query returning null, but it was still coming back 200.

As for the handleError and statusCode priority (that is how I thought of it, which status code takes precedence over other status codes): I left it open to a handleError for that reason. Precedence and priority is left to the user to decide, and whether the user wants to throw an error or not. The server doesn't decide. This became important to our project, because we had queries failing for critical data, but we were still caching them because of the 200 statusCode. This way we could choose how we want to handle the query based on what errors we encountered.

Is the Non-Null fix in place now? I think that returning a 500 for null queries should provide us with something to work with.

[leebyron]

That all makes perfect sense. The fix is in place now with #151. I'll have a release out later in the day for you to use.

Sometimes it makes sense to leave these sorts of things open for the user to decide, but in this case I think it could lead to some strange coupling between the http middleware and your application data specific logic, and might make it difficult to build reliable tools.

The resulting state of things is that as long as data is something other than null, it should be considered a successful (albeit perhaps partially successful) response. Though if data is null, then you'll get a 500.

This lets you model the error behavior you want in your GraphQL type system rather than having to hack it in after the fact at the http boundary. Any fields that are allowed to return null may do so and result in a (partially) valid result, but fields that are non-null will bubble those errors up and if they invalidate the entire request, then you'll appropriately get the 500.

[jakubknejzlik]

Sorry for bringing up closed issues, but I'm not sure if it's good decision to return 500 for any error except the bad graphql query. The status code description is very strict:

The server encountered an unexpected condition which prevented it from fulfilling the request

And I'm not sure if it covers all cases very well. For example these cases IMO should not be reported as server's fault:

• authorization mutation when user provides bad credentials
• user provides input in invalid format (phone number, email address, etc.)
• user requests for query or part of the query he doesn't have access to

Also when working with apollo-client and some other client libraries (eg. Alamofire if I'm not mistaken), they don't parse response body when the server returns error.

I also checked graphql-server and they return 200 even when error occurs (I'm not saying that's correcet behavior :) )

[sorenbs]

@leebyron Now that we are at it :-)

Is it reasonable for a graphql server to return:

• 429 Too Many Requests
• 504 Gateway Time-out

Or should it rather return 500 in both cases and include a more descriptive graphql error?

cc @mavilein