Show beautiful GraphQL error messages

[bkeepers]

GraphQL returns great error messages:

GraphQLError: [{"message":"Resource not accessible by integration","type":"FORBIDDEN","path":["resource","author","hovercard"],"locations":[{"line":11,"column":15}]}]

We should take advantage of that and make them B-E-A-utiful!

There was an error in your GraphQL Query:

on line 11, column 15:
              ... on User {
                hovercard {
                
                ^-- FORBIDDEN: Resource not accessible by integration

We need to get a full spec on what all the GraphQL errors will include, but here's some thoughts:

• If locations is included, then show the original query with a pointer to exactly where the error occurred
• Colorize the output so the error message is in red, and the context is in gray/white
• If there are multiple errors, do this for all of them

cc @cjoudrey @tcbyrd

[cjoudrey]

The GraphQL spec says that all errors part of the "errors" key must have a message.

The locations key is optional as per the spec, but I believe in most cases will be present when using the GitHub API. I would still handle the case where locations isn't present just in case.

The spec allows GraphQL servers to provide extra keys with the error, for example the GitHub API includes "type". This isn't something we've publicly documented yet and may change in the future. I would try not to rely on those for now.

Another note, keep in mind it's very much possible to receive multiple errors for a given query. For example, imagine you request two fields the integration doesn't have access to.

Let me know if there's anything else I can shed light on. 😄

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[tcbyrd]

As discussed in Slack

And also looking back at the spec that was linked above:

If the data entry in the response is not null, the errors entry in the response may contain any errors that occurred during execution. If errors occurred during execution, it should contain those errors.

So per the spec, errors can still be present even when partial data is returned. On the client side (Apollo, Relay), they recommend looking at error types and only presenting ones in the UI that make sense to display to the user. I think it would make sense here to just use log.error for any error messages instead of throwing.

Does GitHub return a different status code if it is able to return a partial response vs not able to respond at all?

Testing this locally, I get GitHub request: POST /graphql - 200 OK for all responses, including queries that return no data and all errors, and ones that return partial data and some errors. This means we can never intentionally throw, since some data can still be returned even though other resources are blocked.

Example:

{
  "data": {
    "resource": {
      "title": "Radar: Weekly Planning for 2018-05-13",
      "createdAt": "2018-05-13T01:00:34Z",
    }
  },
  "errors": [
    {
      "message": "Resource protected by organization SAML enforcement. You must grant your personal token access to this organization.",
      "type": "FORBIDDEN",
      "path": [
        "resource",
        "comments",
        "edges",
        0,
        "node"
      ],
      "locations": [
        {
          "line": 10,
          "column": 11
        }
      ]
    }
  ]
}

Pushing the errors up into the client could be accomplished by only returning res.data (instead of res.data.data), then doing something like this:

const { data, errors } = await context.github.query(getResource, {
      url: context.payload.issue.html_url
    })

if (errors) { context.log.error({ errors }) }

const { resource } = data

It doesn't feel that awesome, but since the errors have types that need to be handled based on the needs of the app, I think we have to push the errors up into the client.

[gr2m]

I think we have to push the errors up into the client.

I agree

[wilhelmklopp]

Given the constraints we're working with it feels like const { data, errors } = awai... isn't that bad a solution. I'm 👍 for that

[gr2m]

the graphql error messages are much more useful now that we use @octokit/graphql. I made a follow up issue for the format Brandon suggested above: octokit/graphql.js#239