Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Standardise the error return from http request #333

Open
jasonkwh opened this issue Apr 10, 2024 · 3 comments · May be fixed by #352
Open

Standardise the error return from http request #333

jasonkwh opened this issue Apr 10, 2024 · 3 comments · May be fixed by #352
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Issues that anyone could pick up and implement if useful to them

Comments

@jasonkwh
Copy link

jasonkwh commented Apr 10, 2024

Is your feature request related to a problem? Please describe.
Hey there. I want this because I need to do a error handling of the gql request. Situations like, if the request is returning 429 error, the program should do a retry, etc. However the current code is returning fmt.Errorf("returned error %v: %s", httpResp.Status, respBody). It is not helping the situation. It makes error handling complicated. Can we return the standard error so that can help the user better handling the errors?

Describe the solution you'd like
We can do a simple enum. Or we can create a new error object includes the http status code + error info (that includes the resp body).

It will be really nice if we can have such feature. Otherwise I will just fork the code and create my derivatives. Thanks.

@jasonkwh jasonkwh added the enhancement New feature or request label Apr 10, 2024
@benjaminjkraft
Copy link
Collaborator

That seems like a sensible thing to add, but I'm not sure offhand what the API would look like. We could add an As-able error type I guess? What do you mean by "the standard error"? Does net/http, or some other stdlib package, have a thing we should be using?

BTW, you can already (a) get direct access to the error and/or (b) retry it by writing your own graphql.Client, or passing a modified http.Client which, say, retries on 429. But I can see how sometimes you would want to know the status code in the application code instead.

@jasonkwh
Copy link
Author

I mean, we can standardise the error messages and return those error messages when something is happening.

On my situation, at the moment i am using strings.prefix to match if there is a 429 error. While implementing the standard error messages, I can just compare the returned error with the gql client provided error. It's a nicer way to handle errors isn't it?

@benjaminjkraft
Copy link
Collaborator

Yeah, looking more there really doesn't seem to be a standard. Ah well, I guess we can make our own As-able error. We already document As-ability to gqlerror.List (and should probably mention it on client.MakeRequest as well) so we can add similar mention there.

Anyway, PRs are welcome! I think this should be a pretty easy one.

@benjaminjkraft benjaminjkraft added good first issue Good for newcomers help wanted Issues that anyone could pick up and implement if useful to them labels Aug 13, 2024
@wwzeng1 wwzeng1 linked a pull request Sep 20, 2024 that will close this issue
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Issues that anyone could pick up and implement if useful to them
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants