PLA-355 - Validating errors ( 400, 401, 404 ) #10
Conversation
PLA-355 Internal errors for invalid AutoPi unit ids
We're seeing 500s from devices-api for Acceptance Criteria:
In the past we had aimed to accomplish this but it seems like the client code was not quite right. |
| errResp := BuildResponseError(res.StatusCode, errors.Errorf("received non success status code %d with body: %s", res.StatusCode, string(body))) | ||
| if res.StatusCode == 400 || res.StatusCode == 401 || res.StatusCode == 404 { | ||
| // unrecoverable since probably bad payload or n | ||
| return retry.Unrecoverable(errResp) | ||
| return retry.Unrecoverable(BuildResponseError(res.StatusCode, errResp)) |
There was a problem hiding this comment.
suggestion (non-blocking): Wrapping this twice seems strange to me. What if we did something like
type HTTPResponseError struct {
StatusCode int
Body []byte
}
func (e *HTTPResponseError) Error() string {
return fmt.Sprintf("status code %d with body %s", e.StatusCode, e.Body)
}
created that, and wrapped if the status code indicates unrecoverable?
There was a problem hiding this comment.
thought: It might also help the discussion here to talk about how you'd want to use this new behavior of HTTPClientWrapper to pass along 404s. Right now it seems like you have to search for the string 404.
http_client_wrapper.go
Outdated
| if _, ok := err.(retry.Error); ok { | ||
| retryErrors := err.(retry.Error) |
There was a problem hiding this comment.
nitpick: I think we can bring this down to one line:
| if _, ok := err.(retry.Error); ok { | |
| retryErrors := err.(retry.Error) | |
| if retryErrors, ok := err.(retry.Error); ok { |
| if _, ok := err.(retry.Error); ok { | ||
| retryErrors := err.(retry.Error) | ||
| for _, e := range retryErrors { | ||
| if httpError, ok := e.(HTTPResponseError); ok { |
There was a problem hiding this comment.
thought: This library is interesting and I hadn't looked at it closely before—if it's not nil the error here is a list of length 5 with all the errors returned. That is cool. I wonder if this is the right thing to do when, for example, we get a 500 and then later in the retry process a transport-level error.
I guess it's hard to summarize five different errors. What's here is good.
http_client_wrapper_test.go
Outdated
| @@ -21,7 +21,7 @@ func Test_httpClientWrapper_ExecuteRequest_failsTooManyRetries(t *testing.T) { | |||
|
|
|||
| assert.Equal(t, 5, c, "expected five retries") | |||
| assert.Error(t, err, "expected error") | |||
| assert.Contains(t, err.Error(), "All attempts fail") | |||
| //assert.Contains(t, myErr, "All attempts fail") | |||
There was a problem hiding this comment.
suggestion: Can we check that the returned error mentions the status code and contains the body?
.gitignore
Outdated
| @@ -13,3 +13,4 @@ | |||
|
|
|||
| # Dependency directories (remove the comment below to include it) | |||
| # vendor/ | |||
| *.idea | |||
There was a problem hiding this comment.
nitpick: Add a newline at the end here.
Proposed Changes
Validating http errors