shurcooL / githubv4

Package githubv4 is a client library for accessing GitHub GraphQL API v4 (https://docs.github.com/en/graphql).
MIT License
1.11k stars 89 forks source link

Provide errors response for error handling #41

Open int128 opened 5 years ago

int128 commented 5 years ago

GitHub GraphQL API returns errors response on error cases such as not found, for example:

{
  "data": {
    "repository": null
  },
  "errors": [
    {
      "type": "NOT_FOUND",
      "path": [
        "repository"
      ],
      "locations": [
        {
          "line": 2,
          "column": 3
        }
      ],
      "message": "Could not resolve to a Repository with the name 'non-existent-repository'."
    }
  ]
}

Currently Query() does not provide any errors but it should do for error handling, for example:

err := client.Query(ctx, &q, vars)
if errors, ok := githubv4.Errors(err); ok {
  if len(errors) > 0 && errors[0].Type() == "NOT_FOUND" {
    // not found case
  }
}

Thank you for the great work!

int128 commented 5 years ago

This is workaround, we can identify if errors is provided by checking pointer type as follows:

var q *struct{}
if err := client.Query(ctx, &q, vars); err != nil {
  if q != nil {
    return errors.Wrapf(err, "this is an error such as not found or invalid query")
  }
  return errors.Wrapf(err, "this is an error such as network or bad json")
}
mweibel commented 5 years ago

👍 I think actually shurcooL/graphql#31 or rather shurcooL/graphql#33 should fix this and this library here doesn't need to change much. Is there anything we can do to help bring this forward?

dmitshur commented 5 years ago

I think actually shurcooL/graphql#31 or rather shurcooL/graphql#33 should fix this and this library here doesn't need to change much.

Agreed.

Is there anything we can do to help bring this forward?

Sorry, I won't have the ability to focus on this for the next few weeks, but I'd like to after that. This is an important API decision, and I'd like to take care to find a good solution rather than rushing.

I suggest iterating on this on forks as a workaround if you need a solution sooner. Reporting results here would be helpful. Thanks.