Improve Do methods

httpclient/client.go

@@ -16,8 +16,7 @@ import (
 
 // Client is the http client implementation
 type Client struct {
-	client heimdall.Doer
-
+	client     heimdall.Doer
 	timeout    time.Duration
 	retryCount int
 	retrier    heimdall.Retriable
@@ -53,66 +52,51 @@ func NewClient(opts ...Option) *Client {
 
 // Get makes a HTTP GET request to provided URL
 func (c *Client) Get(url string, headers http.Header) (*http.Response, error) {
-	var response *http.Response
 	request, err := http.NewRequest(http.MethodGet, url, nil)
 	if err != nil {
-		return response, errors.Wrap(err, "GET - request creation failed")
+		return nil, errors.Wrap(err, "GET - request creation failed")
 	}
-
 	request.Header = headers
-
 	return c.Do(request)
 }
 
 // Post makes a HTTP POST request to provided URL and requestBody
 func (c *Client) Post(url string, body io.Reader, headers http.Header) (*http.Response, error) {
-	var response *http.Response
 	request, err := http.NewRequest(http.MethodPost, url, body)
 	if err != nil {
-		return response, errors.Wrap(err, "POST - request creation failed")
+		return nil, errors.Wrap(err, "POST - request creation failed")
 	}
-
 	request.Header = headers
-
 	return c.Do(request)
 }
 
 // Put makes a HTTP PUT request to provided URL and requestBody
 func (c *Client) Put(url string, body io.Reader, headers http.Header) (*http.Response, error) {
-	var response *http.Response
 	request, err := http.NewRequest(http.MethodPut, url, body)
 	if err != nil {
-		return response, errors.Wrap(err, "PUT - request creation failed")
+		return nil, errors.Wrap(err, "PUT - request creation failed")
 	}
-
 	request.Header = headers
-
 	return c.Do(request)
 }
 
 // Patch makes a HTTP PATCH request to provided URL and requestBody
 func (c *Client) Patch(url string, body io.Reader, headers http.Header) (*http.Response, error) {
-	var response *http.Response
 	request, err := http.NewRequest(http.MethodPatch, url, body)
 	if err != nil {
-		return response, errors.Wrap(err, "PATCH - request creation failed")
+		return nil, errors.Wrap(err, "PATCH - request creation failed")
 	}
-
 	request.Header = headers
-
 	return c.Do(request)
 }
 
 // Delete makes a HTTP DELETE request with provided URL
 func (c *Client) Delete(url string, headers http.Header) (*http.Response, error) {
-	var response *http.Response
 	request, err := http.NewRequest(http.MethodDelete, url, nil)
 	if err != nil {
-		return response, errors.Wrap(err, "DELETE - request creation failed")
+		return nil, errors.Wrap(err, "DELETE - request creation failed")
 	}
-
 	request.Header = headers
-
 	return c.Do(request)
 }
 
@@ -121,43 +105,33 @@ func (c *Client) Do(request *http.Request) (*http.Response, error) {
 	request.Close = true
 
 	var reqBuffer []byte
-
 	if request != nil && request.Body != nil {
 		var err error
-
 		// Storing request buffer to create new reader on each request
 		reqBuffer, err = ioutil.ReadAll(request.Body)
-
 		if err != nil {
 			return nil, err
 		}
 	}
 	multiErr := &valkyrie.MultiError{}
-	var response *http.Response
-
 	for i := 0; i <= c.retryCount; i++ {
-		var err error
 		request.Body = ioutil.NopCloser(bytes.NewBuffer(reqBuffer))
-		response, err = c.client.Do(request)
+		response, err := c.client.Do(request)
 		if err != nil {
 			multiErr.Push(err.Error())
 
 			backoffTime := c.retrier.NextInterval(i)
 			time.Sleep(backoffTime)
 			continue
 		}
-
 		if response.StatusCode >= http.StatusInternalServerError {
 			multiErr.Push(fmt.Sprintf("server error: %d", response.StatusCode))
 
 			backoffTime := c.retrier.NextInterval(i)
 			time.Sleep(backoffTime)
 			continue
 		}
-
-		multiErr = &valkyrie.MultiError{} // Clear errors if any iteration succeeds
-		break
+		return response, nil
 	}
-
-	return response, multiErr.HasError()
+	return nil, multiErr
 }

httpclient/client_test.go

@@ -224,8 +224,7 @@ func TestHTTPClientGetRetriesOnFailure(t *testing.T) {
 	response, err := client.Get(server.URL, http.Header{})
 	require.Error(t, err, "should have failed to make GET request")
 
-	require.Equal(t, http.StatusInternalServerError, response.StatusCode)
-	require.Equal(t, "{ \"response\": \"something went wrong\" }", respBody(t, response))
+	require.Nil(t, response)
 
 	assert.Equal(t, noOfCalls, count)
 }
@@ -255,8 +254,7 @@ func TestHTTPClientGetReturnsAllErrorsIfRetriesFail(t *testing.T) {
 	require.Error(t, err, "should have failed to make GET request")
 
 	require.Equal(t, noOfRetries+1, count)
-	require.Equal(t, http.StatusInternalServerError, response.StatusCode)
-	require.Equal(t, "{ \"response\": \"something went wrong\" }", respBody(t, response))
+	require.Nil(t, response)
 
 	assert.Equal(t, "server error: 500, server error: 500, server error: 500", err.Error())
 }
@@ -327,7 +325,7 @@ func TestHTTPClientGetReturnsErrorOn5xxFailure(t *testing.T) {
 	response, err := client.Get(server.URL, http.Header{})
 	require.Error(t, err, "should have failed to make GET request")
 
-	require.Equal(t, http.StatusInternalServerError, response.StatusCode)
+	require.Nil(t, response)
 
 	assert.Equal(t, "server error: 500", err.Error())
 }

hystrix/hystrix_client.go

@@ -8,15 +8,15 @@ import (
 
 	"github.com/afex/hystrix-go/hystrix"
 	"github.com/gojektech/heimdall"
+	"github.com/gojektech/valkyrie"
 	"github.com/pkg/errors"
 )
 
 type fallbackFunc func(error) error
 
 // Client is the hystrix client implementation
 type Client struct {
-	client heimdall.Doer
-
+	client                 heimdall.Doer
 	timeout                time.Duration
 	hystrixTimeout         time.Duration
 	hystrixCommandName     string
@@ -77,97 +77,82 @@ func NewClient(opts ...Option) *Client {
 
 // Get makes a HTTP GET request to provided URL
 func (hhc *Client) Get(url string, headers http.Header) (*http.Response, error) {
-	var response *http.Response
 	request, err := http.NewRequest(http.MethodGet, url, nil)
 	if err != nil {
-		return response, errors.Wrap(err, "GET - request creation failed")
+		return nil, errors.Wrap(err, "GET - request creation failed")
 	}
-
 	request.Header = headers
-
 	return hhc.Do(request)
 }
 
 // Post makes a HTTP POST request to provided URL and requestBody
 func (hhc *Client) Post(url string, body io.Reader, headers http.Header) (*http.Response, error) {
-	var response *http.Response
 	request, err := http.NewRequest(http.MethodPost, url, body)
 	if err != nil {
-		return response, errors.Wrap(err, "POST - request creation failed")
+		return nil, errors.Wrap(err, "POST - request creation failed")
 	}
-
 	request.Header = headers
-
 	return hhc.Do(request)
 }
 
 // Put makes a HTTP PUT request to provided URL and requestBody
 func (hhc *Client) Put(url string, body io.Reader, headers http.Header) (*http.Response, error) {
-	var response *http.Response
 	request, err := http.NewRequest(http.MethodPut, url, body)
 	if err != nil {
-		return response, errors.Wrap(err, "PUT - request creation failed")
+		return nil, errors.Wrap(err, "PUT - request creation failed")
 	}
-
 	request.Header = headers
-
 	return hhc.Do(request)
 }
 
 // Patch makes a HTTP PATCH request to provided URL and requestBody
 func (hhc *Client) Patch(url string, body io.Reader, headers http.Header) (*http.Response, error) {
-	var response *http.Response
 	request, err := http.NewRequest(http.MethodPatch, url, body)
 	if err != nil {
-		return response, errors.Wrap(err, "PATCH - request creation failed")
+		return nil, errors.Wrap(err, "PATCH - request creation failed")
 	}
-
 	request.Header = headers
-
 	return hhc.Do(request)
 }
 
 // Delete makes a HTTP DELETE request with provided URL
 func (hhc *Client) Delete(url string, headers http.Header) (*http.Response, error) {
-	var response *http.Response
 	request, err := http.NewRequest(http.MethodDelete, url, nil)
 	if err != nil {
-		return response, errors.Wrap(err, "DELETE - request creation failed")
+		return nil, errors.Wrap(err, "DELETE - request creation failed")
 	}
-
 	request.Header = headers
-
 	return hhc.Do(request)
 }
 
 // Do makes an HTTP request with the native `http.Do` interface
 func (hhc *Client) Do(request *http.Request) (*http.Response, error) {
 	request.Close = true
 
-	var response *http.Response
-	var err error
+	multiErr := &valkyrie.MultiError{}
 
 	for i := 0; i <= hhc.retryCount; i++ {
-		err = hystrix.Do(hhc.hystrixCommandName, func() error {
+		var response *http.Response
+		err := hystrix.Do(hhc.hystrixCommandName, func() error {
+			var err error
 			response, err = hhc.client.Do(request)
 			if err != nil {
 				return err
 			}
-
 			if response.StatusCode >= http.StatusInternalServerError {
-				return fmt.Errorf("Server is down: returned status code: %d", response.StatusCode)
+				return fmt.Errorf("server error: %d", response.StatusCode)
 			}
 			return nil
 		}, hhc.fallbackFunc)
 
 		if err != nil {
+			multiErr.Push(err.Error())
+
 			backoffTime := hhc.retrier.NextInterval(i)
 			time.Sleep(backoffTime)
 			continue
 		}
-
-		break
+		return response, nil
 	}
-
-	return response, err
+	return nil, multiErr
 }

hystrix/hystrix_client_test.go

@@ -15,15 +15,6 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-type myHTTPClient struct {
-	client http.Client
-}
-
-func (c *myHTTPClient) Do(request *http.Request) (*http.Response, error) {
-	request.Header.Set("foo", "bar")
-	return c.client.Do(request)
-}
-
 func TestHystrixHTTPClientDoSuccess(t *testing.T) {
 	client := NewClient(
 		WithHTTPTimeout(10*time.Millisecond),
@@ -260,6 +251,7 @@ func TestHystrixHTTPClientPatchSuccess(t *testing.T) {
 
 func TestHystrixHTTPClientRetriesOnFailure(t *testing.T) {
 	count := 0
+	noOfRetries := 3
 	backoffInterval := 1 * time.Millisecond
 	maximumJitterInterval := 1 * time.Millisecond
 
@@ -271,7 +263,7 @@ func TestHystrixHTTPClientRetriesOnFailure(t *testing.T) {
 		WithErrorPercentThreshold(10),
 		WithSleepWindow(100),
 		WithRequestVolumeThreshold(10),
-		WithRetryCount(3),
+		WithRetryCount(noOfRetries),
 		WithRetrier(heimdall.NewRetrier(heimdall.NewConstantBackoff(backoffInterval, maximumJitterInterval))),
 	)
 
@@ -287,10 +279,11 @@ func TestHystrixHTTPClientRetriesOnFailure(t *testing.T) {
 	response, err := client.Get(server.URL, http.Header{})
 	require.Error(t, err)
 
-	assert.Equal(t, 4, count)
+	assert.Equal(t, noOfRetries+1, count)
+
+	require.Nil(t, response)
 
-	assert.Equal(t, http.StatusInternalServerError, response.StatusCode)
-	assert.Equal(t, "{ \"response\": \"something went wrong\" }", respBody(t, response))
+	assert.Equal(t, "server error: 500, server error: 500, server error: 500, server error: 500", err.Error())
 }
 
 func TestHystrixHTTPClientReturnsFallbackFailureWithoutFallBackFunction(t *testing.T) {
@@ -371,6 +364,15 @@ func TestHystrixHTTPClientReturnsFallbackFailureWithAFallBackFunctionWhichReturn
 	assert.Nil(t, err)
 }
 
+type myHTTPClient struct {
+	client http.Client
+}
+
+func (c *myHTTPClient) Do(request *http.Request) (*http.Response, error) {
+	request.Header.Set("foo", "bar")
+	return c.client.Do(request)
+}
+
 func TestCustomHystrixHTTPClientDoSuccess(t *testing.T) {
 	client := NewClient(
 		WithHTTPTimeout(10*time.Millisecond),