GitHub v4 API and non-standard error payloads

[abhinav]

Is your feature request related to a problem? Please describe.

I'm experimenting with using genqlient for querying the GitHub v4 API.
Things work for the most part, except one problem:
GitHub's error objects don't match errors in the GraphQL spec exactly.

Problem

Per the GraphQL spec spec,
error objects include message, locations, and path.
An "extensions" field may contain any other arbitrary information that the
server wants to communicate, e.g. programmatically consumable error codes.

Example

{
  "errors": [
    {
      "message": "Name for character with ID 1002 could not be fetched.",
      "locations": [{ "line": 6, "column": 7 }],
      "path": ["hero", "heroFriends", 1, "name"],
      "extensions": {
        "code": "CAN_NOT_FETCH_BY_ID",
        "timestamp": "Fri Feb 9 14:33:09 UTC 2018"
      }
    }
  ]
}

GitHub's GraphQL API puts the error code in a "type" field of the error objects.
Example:

{
  "data": {
    "repository": null
  },
  "errors": [
    {
      "type": "NOT_FOUND",
      "path": ["repository"],
      "locations": [{"line": 1, "column": 36}],
      "message": "Could not resolve to a Repository with the name ''abhinav/does-not-exist-repo''."
    }
  ]
}

The error is returned in a separate "type" field.
This field is not present on the
underlying gqlerror.List type
so it's never parsed and cannot be accessed.

Reproduction

1. Download GitHub GraphQL schema.

   curl -L https://docs.github.com/public/fpt/schema.docs.graphql > github.graphql

2. Write a query.

   # query.graphql
   query lookupRepositoryID(
     $owner: String!
     $name: String!
   ) {
     repository(owner: $owner, name: $name) {
       id
     }
   }

3. Configure code generation.

   # genqlient.yaml
   schema: github.graphql
   operations:
   - "query.graphql"
   generated: query_generated.go
   package: github

4. Generate code.

   go tool github.com/Khan/genqlient

5. Use generated code

   // query_test.go
   package github
   
   import (
       "fmt"
       "os"
       "testing"
   
       "github.com/Khan/genqlient/graphql"
       "github.com/stretchr/testify/require"
       "github.com/vektah/gqlparser/v2/gqlerror"
       "golang.org/x/oauth2"
   )
   
   func TestLookupRepositoryID(t *testing.T) {
       token := os.Getenv("GITHUB_TOKEN")
       require.NotEmpty(t, token, "token must not be empty")
       httpClient := oauth2.NewClient(t.Context(),
   	    oauth2.StaticTokenSource(&oauth2.Token{AccessToken: token}),
       )
   
       gqlClient := graphql.NewClient("https://api.github.com/graphql", httpClient)
       _, err := lookupRepositoryID(t.Context(), gqlClient, "abhinav", "does-not-exist")
   
       require.Error(t, err)
       var gqlErr gqlerror.List
       require.ErrorAs(t, err, &gqlErr)
   
       fmt.Printf("%#v\n", gqlErr[0])
       // Cannot access "type" of the error.
   }

The call fails as expected by the test
but there's no way to access the underlying error "type".

That error codes are communicated like this can also be verified in the
GraphQL client used by gh CLI extensions
(a fork of shurcooL/graphql).

I recognize that this is a weirdness of GitHub's implementation of GraphQL,
but strictly speaking, it is permitted by the spec:

GraphQL services should not provide any additional entries to the error
format since they could conflict with additional entries that may be added in
future versions of this specification.

Note: Previous versions of this spec did not describe the extensions entry
for error formatting.
While non-specified entries are not violations, they are still discouraged.

(Emphasis mine.)

So I'm wondering whether there's a way for genqlient to support this.

Describe the solution you'd like

Currently, genqlient uses gqlerror.List
from vektah/gqlparser for the error response.

Any solution here will likely require a custom error type (exported or otherwise).
I'll cover those below but first, let's talk about backwards compatibility:
the custom error type will want to implement As(any) bool and Is(any) bool
to control the behavior of errors.Is and errors.As.
The implementations will convert the error to gqlerror.List or gqlerror.Error,
or compare against them as needed, so that existing code that consumes gqlerror
does not break.

A couple possible solutions:

1. First-class support for "type"

   genqlient will export a type Error and type ErrorList.
   Error will have a Type string field.

    type Error struct {
      Message string
      Path ast.Path
      Locations []Location
   +  Type string `json:"type"`

   Pros:

   • Solves the problem.

   Cons:

   • GitHub-specific solution.
     Won't generalize to other similarly discouraged (but spec-compliant)
     server implementations.

2. Untyped error data

   genqlient will export a type Error and type ErrorList.
   Error will have the standard Message, Path, Locations, and Extensions field,
   and a new Data map[string]any field that contains any JSON field received
   that are not any of the others.

    type Error struct {
      Message string
      Path ast.Path
      Locations []Location
   +  Data map[string]any

   (This will obviously need custom MarshalJSON and UnmarshalJSON methods
   that handle handling the JSON bag of values correctly.)

   GitHub v4 consumers will do something like:

   var (
       gqlErrs graphql.ErrorList
       errs []error
   )
   if errors.As(err, &gqlErrs) {
       for _, gqlErr := range gqlErrs {
           typ, ok := gqlErr.Data["type"].(string)
           if ok && typ == "NOT_FOUND" {
               errs = append(errs, ErrNotFound)
           }
       }
   }

   Pros:

   • General solution for GitHub and non-GitHub cases.
     Will handle any such spec-compliant non-standard error responses.

   Cons:

   • Not type safe.
   • Fairly complex Error type is now part of genqlient's public API.
     It's likely to leak into downstream code, and is easy to break.

3. User-customizable error shape

   genqlient will support a new configuration option
   to change the error list type in use.
   This will default to gqlerror.List,
   but the user will be able to specify their own.

   # error_list_type is the type used for the list of errors
   # returned in a GraphQL response.
   #
   # The type specified here MUST implement the error interface.
   error_list_type: "github.com/example/foo.githubv4ErrorList"

   type githubv4ErrorList []struct {
       Message string
       Path []any
       Locations []struct{ Line, Column int }
       Type string
       Extensions map[string]any
   }

   Implementation-wise, this can probably be done with a type-parameterized
   Response type, e.g.

   type GResponse[Data any, Errors error] struct { // suggestions for better names?
       Data       Data
       Extensions map[string]any
       Errors     Errors
   }
   
   type Response[T any] = GResponse[T, gqlerror.List] // needs Go >= 1.23

   (I haven't fully thought this through; this may require separate
   error_type and error_list_type configuration options.
   Or we might be able to use only an error_type + an ErrorList[E]
   situation.)

   Pros:

   • General solution for GitHub and non-GitHub cases.
     Will handle any such spec-compliant non-standard error responses.
   • Can be as type-safe or type-unsafe as the user prefers.

   Cons:

   • Users must add Message, Path, Locations, and Extensions themselves.
   • Requires raising minimum required Go version to 1.23 to use generic type alias.

Describe alternatives you've considered

While I lean towards solution (3), I'd love to hear from maintainers
whether this is something worth solving in genqlient,
and whether there is a better design approach here.

One workaround I've considered (and used with shurcooL/githubv4)
is to intercept the response payload before the GraphQL client has a go at it,
and parse the error codes out there myself.
Implementation here.
If y'all decide that this isn't something worth supporting in genqlient,
that's probably the route I'll try to take but it smells a bit to parse the payload twice.

[benjaminjkraft]

Thanks for raising this!

Generally I agree configuring an error type along the lines of (3) seems to make the most sense. Users could embed the standard gqlerrror.List type in theirs if desired. Go 1.24 is out, so we can require Go 1.23 (if you do, keep that as a separate PR, see e.g. #262).

I think we will run into a slight issue here (see #366):

genqlient/graphql/client.go

Line 273 in 35bfe03

Errors: gqlerror.List{&gqlerror.Error{Message: string(respBody)}},

But I'm not sure if that branch of the behavior there is that helpful; if the response body isn't even valid JSON (or isn't readable at all) it's not really a GraphQL error so we could have a separate field of HTTPError we put it in and I'm guessing the author's intention was mainly when you have a non-200 but it is valid JSON. (@HaraldNordgren any thoughts? I guess to preserve compat we could, by a type switch, set the old field only if it exists? I'm not sure if that's more or less confusing. Or have HTTPError be parameterized over your error type, but I think that will break people's Asing if they're not careful so it may be more confusing.)

Along similar lines to what you say, you could also do this via a custom graphql.Client, but I agree it makes sense for us to support this.

[abhinav]

Finally got around to poking at this again.

I don't see a backwards compatible way to do this:
gqlerror.List is just too interwoven in all the logic.
MakeRequest and HTTPError both use Response,
and Response is hard-coded to gqlerror.List.

Relatedly, regarding this:

you could also do this via a custom graphql.Client

Unfortunately, I don't think that'll work.
The graphql.Client interface's MakeRequest method expects a Response,
which requires the errors field to be a gqlerror.List.

Trying it a bit, even if a backwards compatible change was digestible,
doing it with type parameters seems less than ideal
as it leaks the type parameters down into everything: HTTPError, Client, etc.
It hurts usability more than the value it provides.

I think this might be better solved with plain interfaces (and maybe a small amount of reflection).
A less disruptive (but still backwards incompatible) version might turn Errors field on BaseResponse
into an ErrorList interface with the functionality genqlient needs when it touches the list directly.
The field would still be expected to be JSON serializable and deserializable.
The type would be required at client construction (so an instance can exist during deserialization).
If necessary, type casting on the user facing side can be reduced by the code generator.

I'm going to hold off on pursuing this for now, but sharing my findings for anyone else who has interest in this.