Standardize error return from HTTP requests (#363)

This makes the HTTP request errors `As`-able, so you can retry on rate
limit errors, or whatever.

Fixes #333, replaces #352 (I just added docs).

I have:
- [x] Written a clear PR title and description (above)
- [x] Signed the [Khan Academy CLA](https://www.khanacademy.org/r/cla)
- [x] Added tests covering my changes, if applicable
- [x] Included a link to the issue fixed, if applicable
- [x] Included documentation, for new features
- [x] Added an entry to the changelog

---------

Co-authored-by: William Zeng <[email protected]>

docs/CHANGELOG.md

@@ -31,6 +31,7 @@ When releasing a new version:
   See the [documentation](FAQ.md) for how to `subscribe to an API 'subscription' endpoint`.
 - genqlient now supports double-star globs for schema and query files; see [`genqlient.yaml` docs](genqlient.yaml) for more.
 - genqlient now generates slices containing all enum values for each enum type.
+- genqlient now returns `Is`/`As`-able errors when the HTTP request returns a non-200 status.
 
 ### Bug fixes:
 

docs/client_config.md

@@ -119,7 +119,18 @@ For more on accessing response objects for interfaces and fragments, see the [op
 
 ### Handling errors
 
-In addition to the response-struct, each genqlient-generated helper function returns an error.  The response-struct will always be initialized (never nil), even on error.  If the request returns a valid GraphQL response containing errors, the returned error will be [`As`-able](https://pkg.go.dev/errors#As) as [`gqlerror.List`](https://pkg.go.dev/github.com/vektah/gqlparser/v2/gqlerror#List), and the struct may be partly-populated (if one field failed but another was computed successfully).  If the request fails entirely, the error will be another error (e.g. a [`*url.Error`](https://pkg.go.dev/net/url#Error)), and the response will be blank (but still non-nil).
+In addition to the response-struct, each genqlient-generated helper function returns an error. The error may be [`As`-able][As] to one of the following:
+
+- [`gqlerror.List`][gqlerror], if the request returns a valid GraphQL response containing errors; in this case the struct may be partly-populated 
+- [`graphql.HTTPError`][HTTPError], if there was a valid but non-200 HTTP response
+- another error (e.g. a [`*url.Error`][urlError])
+
+In case of a GraphQL error, the response-struct may be partly-populated (if one field failed but another was computed successfully). In other cases it will be blank, but it will always be initialized (never nil), even on error.
+
+[As]: https://pkg.go.dev/errors#As
+[gqlerror]: https://pkg.go.dev/github.com/vektah/gqlparser/v2/gqlerror#List
+[HTTPError]: https://pkg.go.dev/github.com/Khan/genqlient/graphql#HTTPError
+[urlError]: https://pkg.go.dev/net/url#Error
 
 For example, you might do one of the following:
 ```go

graphql/client.go

@@ -244,7 +244,10 @@ func (c *client) MakeRequest(ctx context.Context, req *Request, resp *Response)
 		if err != nil {
 			respBody = []byte(fmt.Sprintf("<unreadable: %v>", err))
 		}
-		return fmt.Errorf("returned error %v: %s", httpResp.Status, respBody)
+		return &HTTPError{
+			StatusCode: httpResp.StatusCode,
+			Body:       string(respBody),
+		}
 	}
 
 	err = json.NewDecoder(httpResp.Body).Decode(resp)

graphql/client_test.go

@@ -0,0 +1,97 @@
+package graphql
+
+import (
+	"context"
+	"encoding/json"
+	"errors"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestMakeRequest_HTTPError(t *testing.T) {
+	testCases := []struct {
+		name               string
+		serverResponseBody string
+		expectedErrorBody  string
+		serverResponseCode int
+		expectedStatusCode int
+	}{
+		{
+			name:               "400 Bad Request",
+			serverResponseBody: "Bad Request",
+			expectedErrorBody:  "Bad Request",
+			serverResponseCode: http.StatusBadRequest,
+			expectedStatusCode: http.StatusBadRequest,
+		},
+		{
+			name:               "429 Too Many Requests",
+			serverResponseBody: "Rate limit exceeded",
+			expectedErrorBody:  "Rate limit exceeded",
+			serverResponseCode: http.StatusTooManyRequests,
+			expectedStatusCode: http.StatusTooManyRequests,
+		},
+		{
+			name:               "500 Internal Server Error",
+			serverResponseBody: "Internal Server Error",
+			expectedErrorBody:  "Internal Server Error",
+			serverResponseCode: http.StatusInternalServerError,
+			expectedStatusCode: http.StatusInternalServerError,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+				w.WriteHeader(tc.serverResponseCode)
+				_, err := w.Write([]byte(tc.serverResponseBody))
+				if err != nil {
+					t.Fatalf("Failed to write response: %v", err)
+				}
+			}))
+			defer server.Close()
+
+			client := NewClient(server.URL, server.Client())
+			req := &Request{
+				Query: "query { test }",
+			}
+			resp := &Response{}
+
+			err := client.MakeRequest(context.Background(), req, resp)
+
+			assert.Error(t, err)
+			var httpErr *HTTPError
+			assert.True(t, errors.As(err, &httpErr), "Error should be of type *HTTPError")
+			assert.Equal(t, tc.expectedStatusCode, httpErr.StatusCode)
+			assert.Equal(t, tc.expectedErrorBody, httpErr.Body)
+		})
+	}
+}
+
+func TestMakeRequest_Success(t *testing.T) {
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusOK)
+		err := json.NewEncoder(w).Encode(map[string]interface{}{
+			"data": map[string]string{
+				"test": "success",
+			},
+		})
+		if err != nil {
+			t.Fatalf("Failed to encode response: %v", err)
+		}
+	}))
+	defer server.Close()
+
+	client := NewClient(server.URL, server.Client())
+	req := &Request{
+		Query: "query { test }",
+	}
+	resp := &Response{}
+
+	err := client.MakeRequest(context.Background(), req, resp)
+
+	assert.NoError(t, err)
+	assert.Equal(t, map[string]interface{}{"test": "success"}, resp.Data)
+}

graphql/errors.go

@@ -0,0 +1,14 @@
+package graphql
+
+import "fmt"
+
+// HTTPError represents an HTTP error with status code and response body.
+type HTTPError struct {
+	Body       string
+	StatusCode int
+}
+
+// Error implements the error interface for HTTPError.
+func (e *HTTPError) Error() string {
+	return fmt.Sprintf("returned error %v: %s", e.StatusCode, e.Body)
+}

internal/integration/integration_test.go

@@ -7,13 +7,15 @@ package integration
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"net/http"
 	"testing"
 	"time"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"github.com/vektah/gqlparser/v2/gqlerror"
 
 	"github.com/Khan/genqlient/graphql"
 	"github.com/Khan/genqlient/internal/integration/server"
@@ -159,6 +161,15 @@ func TestServerError(t *testing.T) {
 		// response -- and indeed in this case it should even have another field
 		// (which didn't err) set.
 		assert.Error(t, err)
+		t.Logf("Full error: %+v", err)
+		var gqlErrors gqlerror.List
+		if !assert.True(t, errors.As(err, &gqlErrors), "Error should be of type gqlerror.List") {
+			t.Logf("Actual error type: %T", err)
+			t.Logf("Error message: %v", err)
+		} else {
+			assert.Len(t, gqlErrors, 1, "Expected one GraphQL error")
+			assert.Equal(t, "oh no", gqlErrors[0].Message)
+		}
 		assert.NotNil(t, resp)
 		assert.Equal(t, "1", resp.Me.Id)
 	}
@@ -176,6 +187,8 @@ func TestNetworkError(t *testing.T) {
 		//	return resp.Me.Id, err
 		// without a bunch of extra ceremony.
 		assert.Error(t, err)
+		var gqlErrors gqlerror.List
+		assert.False(t, errors.As(err, &gqlErrors), "Error should not be of type gqlerror.List for network errors")
 		assert.NotNil(t, resp)
 		assert.Equal(t, new(failingQueryResponse), resp)
 	}