Improve error handling, expose error and error_description #108
Conversation
internal/workos/http.go
Outdated
| Status string | ||
| RequestID string | ||
| Message string | ||
| Err string |
There was a problem hiding this comment.
Can we expand these names to Error and ErrorDescription?
There was a problem hiding this comment.
@maxdeviant I'm looking at it now and just remembered why I did the naming this way 😅 . It seems that Error might be a reserved name in this case, as using it seems to introduce a slew of errors:
./http.go:37:18: HTTPError.Error is a field, not a method
./http.go:37:18: cannot use HTTPError{...} (type HTTPError) as type error in return argument:
HTTPError does not implement error (missing Error method)
./http.go:57:6: type HTTPError has both field and method named Error
Is there a different nomenclature that might work? Shouldn't be an issue with ErrorDescription.
internal/workos/http.go
Outdated
| } | ||
|
|
||
| func (e HTTPError) Error() string { | ||
| return fmt.Sprintf("%s: request id %q: %s", e.Status, e.RequestID, e.Message) | ||
| return fmt.Sprintf("%s: request id %q: %s %s %s", e.Status, e.RequestID, e.Message, e.Err, e.ErrDescription) |
There was a problem hiding this comment.
Some of these properties are going to be mutually-exclusive (for example, we won't have a message alongside an error and error_description.
We should probably adjust the string representation based on which properties are present.
internal/workos/http.go
Outdated
| @@ -25,23 +27,39 @@ func TryGetHTTPError(r *http.Response) error { | |||
| } else { | |||
| msg = string(body) | |||
| } | |||
| if e := gjson.GetBytes(body, "error").Str; e != "" { | |||
There was a problem hiding this comment.
We removed gjson as a dependency in #115, so we'll have to adjust our approach.
There was a problem hiding this comment.
@maxdeviant I've been working on this but can't seem to use your logic to accomplish the same behavior I was getting through gjson.GetBytes(body, "error") or gjson.GetBytes(body, "error_description")
If you have any pointers, I'd appreciate it.
There was a problem hiding this comment.
For example, just attempting to use the same logic and extract error description doesn't seem to work:
func getJsonErrorDescription(b []byte) string {
var response struct{ ErrorDescription string }
if err := json.Unmarshal(b, &response); err != nil {
return ""
}
return response.ErrorDescription
}
The previous implementation worked, but relied on the JSON parser failing on plain-text responses. Seems better to be explicit about when we decide to treat the response as JSON.
`r` is already being used for `http.Response` in this file.
|
I think I have this working correctly again, without the old |
In the rare case that we send back invalid JSON with `Content-Type` set as JSON, it'll probably be more useful for the developer to see the entire body instead of the decoding error message.
internal/workos/http_test.go
Outdated
| @@ -1,18 +1,37 @@ | |||
| package workos | |||
|
|
|||
| import ( | |||
| "github.com/stretchr/testify/require" | |||
There was a problem hiding this comment.
nit: Not sure if this was a gofmt change or a manual one, but AFAIK it's idiomatic to keep the imports for the standard library separate from external imports.
Separate internal from external dependencies
logangingerich
deleted the
feature/sdk-66-improve-error-handling-in-go-sdk
branch
https://linear.app/workos/issue/SDK-66/improve-error-handling-in-go-sdk
Exposes error and error_description in error body to improve error handling.