graphql icon indicating copy to clipboard operation
graphql copied to clipboard

RFC: less opaque error type

Open mspiegel opened this issue 7 years ago • 2 comments

I want to submit a pull request that provides a less opaque error type. This is similar to #29 but has a broader purpose. I want to get some feedback before I submit a pull request.

  1. Can I export the errors type? That will allow me to do something with the Message and Locations.

  2. How do I provide more information about where Client.do() is generating the error? I see 6 locations where an error is returned. Which is a better approach: a) Should I declare a different error type for each of these conditions? Is that the idiomatic Go way? That appears to be what the golang standard library does. b) Should I use an enum and wrap the error like this https://play.golang.org/p/xPca1LmOBbp? One advantage of this approach is that (unlike option a) we aren't declaring a new error type that is just some wrapper for another error type (like a json error type). The enum describes where the error occurs not the shape of the error. c) Something else?

mspiegel avatar Sep 25 '18 15:09 mspiegel

Hi @mspiegel, thank you for opening an issue to discuss this first, it's very helpful.

Yes, a less opaque error type makes sense as a next step to consider. I already alluded to it in https://github.com/shurcooL/graphql/issues/29#issuecomment-423875439:

A bigger step might be to create a struct error type with a ResponseBody []byte field in it, but I'd rather defer that until it's really needed.

I'm happy to proceed discussing this, but I would like to gather some data on the use cases first. These might seem like obvious questions, but I want to ask them anyway. Who would use this? How would they benefit? Are these situations hypothetical, or have you actually encountered them?

I would like to implement this only if it solves real problems. Understanding what those are will help us evaluate potential solutions and make sure we pick a good one.

So, can you please elaborate on why you want this, and what can't be done right now (or can only be done inconveniently).

dmitshur avatar Sep 26 '18 05:09 dmitshur

Sure. I'm building a GitHub integration. So it will listen for events from GitHub, it may perform actions based on those events, and then it returns an http response.

Incidentally I don't know what is the best practice on sync vs async GitHub integrations. Sync makes it easier to debug because the webhook responses show up in the GitHub UI. I've informally heard that GitHub prefers async to avoid timeouts and also their servers don't want to wait for my service to complete.

Let's assume I continue with sync responses. In cases of an error I need to return a message body and an http status code. When I was using the GitHub V3 API then their status code became my status code. This works out remarkably well. It has the additional benefit that I can treat 400-level responses as warnings that don't require immediate attention. And 500-level responses as errors that will trigger a notification to me.

I don't really have that nice demarcation of error types in the GitHub v4 API. With the current error handling in this library, I get even less information back than I could be getting. As a first implementation, I think that I could treat any GraphQL error responses (the errors array type) as warnings. And any other kind of error, like JSON serde errors, as errors. I might be able to take advantage of the http response code although it's not defined when GitHub returns a non-200 response.

mspiegel avatar Sep 26 '18 11:09 mspiegel