fix: return response body on error status code (#186)

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.

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()
+		})
+	}
+}

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

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",

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
+	}
+}