From f4be6fac76b9f5973131a3eb051945a22f0dd99c Mon Sep 17 00:00:00 2001 From: Murad Biashimov Date: Wed, 20 Nov 2024 15:33:36 +0100 Subject: [PATCH] fix: return response body on error status code Uses retryablehttp.PassthroughErrorHandler to return response body as an error. By default retryablehttp returns attempts information instead. --- client.go | 84 +++++++++++++++++++++++++++----------------------- client_test.go | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++ error.go | 15 +++++---- error_test.go | 4 +-- option.go | 23 ++++++++++++++ 5 files changed, 163 insertions(+), 47 deletions(-) diff --git a/client.go b/client.go index daae386..05587cf 100644 --- a/client.go +++ b/client.go @@ -39,44 +39,29 @@ func NewClient(opts ...Option) (Client, error) { return nil, err } - c := retryablehttp.NewClient() - - // Retry settings explanation: - // Default values (retryWaitMin = 1s, retryWaitMax = 30s, retryMax = 4) - // Changed values (retryWaitMin = 2s, retryWaitMax = 15s, retryMax = 6) - // - // Default values | Changed values - // Run Seconds | Run Seconds - // 0 0.000 | 0 0.000 - // 1 1.000 | 1 2.000 - // 2 3.000 | 2 6.000 - // 3 7.000 | 3 14.000 - // 4 15.000 | 4 15.000 (capped at retryWaitMax) - // | 5 15.000 (capped at retryWaitMax) - // | 6 15.000 (capped at retryWaitMax) - // - // Maximum wait time if all attempts fail: - // Default values: 26 seconds - // Changed values: 67 seconds - const ( - //nolint:revive - retryWaitMin = 2 * time.Second - retryWaitMax = 15 * time.Second - retryMax = 6 - ) - c.RetryWaitMin = retryWaitMin // Default is 1 second - c.RetryWaitMax = retryWaitMax // Default is 30 seconds - c.RetryMax = retryMax // Default is 4 retries (5 attempts in total) - c.CheckRetry = checkRetry - - c.Logger = nil - d.doer = c.StandardClient() - // User settings for _, opt := range opts { opt(d) } + // When DoerOpt is not applied + if d.doer == nil { + c := retryablehttp.NewClient() + c.RetryMax = d.RetryMax + c.RetryWaitMin = d.RetryWaitMin + c.RetryWaitMax = d.RetryWaitMax + c.CheckRetry = checkRetry + + // Disables retryablehttp logger which outputs a lot of debug information + c.Logger = nil + + // By default, when retryablehttp gets 500 (or any error), + // it doesn't return the body which might contain useful information. + // Instead, it returns `giving up after %d attempt(s)` for the given url and method. + c.ErrorHandler = retryablehttp.PassthroughErrorHandler + d.doer = c.StandardClient() + } + if d.Debug { out := zerolog.ConsoleWriter{Out: os.Stderr, TimeFormat: time.RFC3339} d.logger = zerolog.New(out).With().Timestamp().Logger() @@ -99,13 +84,34 @@ func NewClient(opts ...Option) (Client, error) { return newClient(d), nil } +// Retry settings explanation: +// Default values (retryWaitMin = 1s, retryWaitMax = 30s, retryMax = 4) +// Changed values (retryWaitMin = 2s, retryWaitMax = 15s, retryMax = 6) +// +// Default values | Changed values +// Run Seconds | Run Seconds +// 0 0.000 | 0 0.000 +// 1 1.000 | 1 2.000 +// 2 3.000 | 2 6.000 +// 3 7.000 | 3 14.000 +// 4 15.000 | 4 15.000 (capped at retryWaitMax) +// +// | 5 15.000 (capped at retryWaitMax) +// | 6 15.000 (capped at retryWaitMax) +// +// Maximum wait time if all attempts fail: +// Default values: 26 seconds +// Changed values: 67 seconds type aivenClient struct { - Host string `envconfig:"AIVEN_WEB_URL" default:"https://api.aiven.io"` - UserAgent string `envconfig:"AIVEN_USER_AGENT" default:"aiven-go-client/v3"` - Token string `envconfig:"AIVEN_TOKEN"` - Debug bool `envconfig:"AIVEN_DEBUG"` - logger zerolog.Logger - doer Doer + Host string `envconfig:"AIVEN_WEB_URL" default:"https://api.aiven.io"` + UserAgent string `envconfig:"AIVEN_USER_AGENT" default:"aiven-go-client/v3"` + Token string `envconfig:"AIVEN_TOKEN"` + Debug bool `envconfig:"AIVEN_DEBUG"` + RetryMax int `envconfig:"AIVEN_CLIENT_RETRY_MAX" default:"6"` + RetryWaitMin time.Duration `envconfig:"AIVEN_CLIENT_RETRY_WAIT_MIN" default:"2s"` + RetryWaitMax time.Duration `envconfig:"AIVEN_CLIENT_RETRY_WAIT_MAX" default:"15s"` + logger zerolog.Logger + doer Doer } // OperationIDKey is the key used to store the operation ID in the context. diff --git a/client_test.go b/client_test.go index bda978f..9d9b0c2 100644 --- a/client_test.go +++ b/client_test.go @@ -10,6 +10,7 @@ import ( "strings" "sync/atomic" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -114,3 +115,86 @@ func TestServiceCreate(t *testing.T) { // All calls are received assert.EqualValues(t, 2, callCount) } + +func TestServiceCreateErrorsRetries(t *testing.T) { + cases := []struct { + Name string + ResponseBody string + ErrorExpect string + RetryMax int + CallsExpect int + }{ + { + Name: "message field only", + ResponseBody: `{"message": "Internal Server Error"}`, + ErrorExpect: "[500 ServiceCreate]: Internal Server Error", + RetryMax: 6, + CallsExpect: 7, + }, + { + Name: "with errors field, list", + ResponseBody: `{"message": "Something went wrong", "errors": ["oh!", "no!"]}`, + ErrorExpect: `[500 ServiceCreate]: Something went wrong: ["oh!","no!"]`, + RetryMax: 1, + CallsExpect: 2, + }, + { + Name: "with errors field, string", + ResponseBody: `{"message": "Something went wrong", "errors": "wow!"}`, + ErrorExpect: `[500 ServiceCreate]: Something went wrong: "wow!"`, + RetryMax: 1, + CallsExpect: 2, + }, + } + + for _, tt := range cases { + t.Run(tt.Name, func(t *testing.T) { + var callsActual int64 + + // Creates a test server + mux := http.NewServeMux() + mux.HandleFunc( + "/v1/project/aiven-project/service", + func(w http.ResponseWriter, _ *http.Request) { + // Creates response + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusInternalServerError) + _, err := w.Write([]byte(tt.ResponseBody)) + require.NoError(t, err) + atomic.AddInt64(&callsActual, 1) + }, + ) + + server := httptest.NewServer(mux) + + // Points a new client to the server url + c, err := NewClient( + TokenOpt("token"), + UserAgentOpt("unit-test"), + HostOpt(server.URL), + RetryMaxOpt(tt.RetryMax), + RetryWaitMinOpt(1*time.Millisecond), + RetryWaitMaxOpt(3*time.Millisecond), + DebugOpt(false), + ) + require.NotNil(t, c) + require.NoError(t, err) + + // Makes create request + in := &service.ServiceCreateIn{ + ServiceName: "my-clickhouse", + ServiceType: "clickhouse", + } + + ctx := context.Background() + project := "aiven-project" + out, err := c.ServiceCreate(ctx, project, in) + assert.Nil(t, out) + assert.Equal(t, err.Error(), tt.ErrorExpect) + + // All calls are received + assert.EqualValues(t, tt.CallsExpect, callsActual) + server.Close() + }) + } +} diff --git a/error.go b/error.go index cd2c486..560d702 100644 --- a/error.go +++ b/error.go @@ -23,16 +23,19 @@ type Error struct { // Error concatenates all the fields. func (e Error) Error() string { - var errMerged []byte - var err error - errMerged, err = json.Marshal(e.Errors) + // Must not use `%q` here which will escape every quote in the string, + // which might break external substring checks + msg := fmt.Sprintf(`[%d %s]: %s`, e.Status, e.OperationID, e.Message) + if e.Errors == nil { + return msg + } + + errMerged, err := json.Marshal(e.Errors) if err != nil { errMerged = []byte(err.Error()) } - // Must not use `%q` here which will escape every quote in the string. - // It might break external substring checks - return fmt.Sprintf(`[%d %s]: %s: %s`, e.Status, e.OperationID, e.Message, errMerged) + return fmt.Sprintf(`%s: %s`, msg, errMerged) } // IsNotFound returns true if the specified error has status 404 diff --git a/error_test.go b/error_test.go index feea77c..157df4d 100644 --- a/error_test.go +++ b/error_test.go @@ -159,7 +159,7 @@ func TestFromResponse(t *testing.T) { statusCode: http.StatusInternalServerError, body: []byte(`unknown error`), expectBytes: nil, - expectStringer: `[500 UserAuth]: unknown error: null`, + expectStringer: `[500 UserAuth]: unknown error`, expectErr: Error{ OperationID: "UserAuth", Message: "unknown error", @@ -171,7 +171,7 @@ func TestFromResponse(t *testing.T) { operationID: "UserAuth", statusCode: http.StatusBadRequest, body: []byte(`{"message": {"key": "value"}}`), // message is not string - expectStringer: `[400 UserAuth]: json: cannot unmarshal object into Go struct field Error.message of type string: null`, + expectStringer: `[400 UserAuth]: json: cannot unmarshal object into Go struct field Error.message of type string`, expectBytes: nil, expectErr: Error{ OperationID: "UserAuth", diff --git a/option.go b/option.go index 3eb3801..d9fd415 100644 --- a/option.go +++ b/option.go @@ -1,6 +1,8 @@ // Package aiven provides a client for interacting with the Aiven API. package aiven +import "time" + // Option is a function that configures the client. type Option func(*aivenClient) @@ -39,3 +41,24 @@ func DoerOpt(doer Doer) Option { d.doer = doer } } + +// RetryMaxOpt sets the maximum number of retries +func RetryMaxOpt(retryMax int) Option { + return func(d *aivenClient) { + d.RetryMax = retryMax + } +} + +// RetryWaitMinOpt sets the minimum wait time between retries +func RetryWaitMinOpt(retryWaitMin time.Duration) Option { + return func(d *aivenClient) { + d.RetryWaitMin = retryWaitMin + } +} + +// RetryWaitMaxOpt sets the maximum wait time between retries +func RetryWaitMaxOpt(retryWaitMax time.Duration) Option { + return func(d *aivenClient) { + d.RetryWaitMax = retryWaitMax + } +}