Standardize error return from HTTP requests #363
Conversation
benjaminjkraft
requested review from
csilvers,
dnerdy and
StevenACoffman
as code owners
|
|
||
| // Error implements the error interface for HTTPError. | ||
| func (e *HTTPError) Error() string { | ||
| return fmt.Sprintf("returned error %v: %s", e.StatusCode, e.Body) |
There was a problem hiding this comment.
This error-text can be hard to parse when the body is the empty string (or just whitespace), so you may want to do %#v instead of %s, or something like >>>%s<<<. Though maybe it's not going to matter much in practice.
There was a problem hiding this comment.
I think it needs to be %s to convert the bytes to string, but I can at least add quotes!
Thanks to Craig for the suggestion! Since people may have, before #363, been parsing the text, I mentioned it as a breaking change, although I imagine most people are parsing just the code.
Thanks to Craig for the suggestion! Since people may have, before #363, been parsing the text, I mentioned it as a breaking change, although I imagine most people are parsing just the code, not the text. 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
|
Hi @benjaminjkraft and @csilvers ! This is really nice, and I wonder if we can go further still? This is how my logic was changed to fit with these latest changes: - expected: errors.New("returned error 422 Unprocessable Entity: {\"errors\":[{\"message\":\" is not a valid MealType\",\"path\":[\"variable\",\"request\",\"mealType\"],\"extensions\":{\"code\":\"GRAPHQL_VALIDATION_FAILED\"}}],\"data\":null}"),
+ expected: &graphql.HTTPError{
+ Body: "{\"errors\":[{\"message\":\" is not a valid MealType\",\"path\":[\"variable\",\"request\",\"mealType\"],\"extensions\":{\"code\":\"GRAPHQL_VALIDATION_FAILED\"}}],\"data\":null}",
+ StatusCode: 422,
+ },If we go one step further, should be able to expose the underlying type BaseResponse[T any] struct {
Data T `json:"data"`
Extensions map[string]interface{} `json:"extensions,omitempty"`
Errors gqlerror.List `json:"errors,omitempty"`
}
type Response BaseResponse[any]Notice that the structure of the data is my diff already matches exactly with PR for a proposed implementation: #366 |
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: