adding WithRetry option (bold-commerce#87)

* adding WithRetry option

* refactor usagecharge test, added go 1.14 and bionic (bold-commerce#97)

Thank you very much for this contribution.

goshopify.go

@@ -72,6 +72,10 @@ type Client struct {
 	// A permanent access token
 	token string
 
+	// max number of retries, defaults to 0 for no retries see WithRetry option
+	retries  int
+	attempts int
+
 	RateLimits RateLimitInfo
 
 	// Services used for communicating with the API
@@ -215,21 +219,6 @@ func (c *Client) NewRequest(method, relPath string, body, options interface{}) (
 	return req, nil
 }
 
-// Option is used to configure client with options
-type Option func(c *Client)
-
-// WithVersion optionally sets the api-version if the passed string is valid
-func WithVersion(apiVersion string) Option {
-	return func(c *Client) {
-		pathPrefix := defaultApiPathPrefix
-		if len(apiVersion) > 0 && apiVersionRegex.MatchString(apiVersion) {
-			pathPrefix = fmt.Sprintf("admin/api/%s", apiVersion)
-		}
-		c.apiVersion = apiVersion
-		c.pathPrefix = pathPrefix
-	}
-}
-
 // NewClient returns a new Shopify API client with an already authenticated shopname and
 // token. The shopName parameter is the shop's myshopify domain,
 // e.g. "theshop.myshopify.com", or simply "theshop"
@@ -308,17 +297,54 @@ func (c *Client) Do(req *http.Request, v interface{}) error {
 
 // doGetHeaders executes a request, decoding the response into `v` and also returns any response headers.
 func (c *Client) doGetHeaders(req *http.Request, v interface{}) (http.Header, error) {
-	resp, err := c.Client.Do(req)
-	if err != nil {
-		return nil, err
-	}
-	defer resp.Body.Close()
+	var resp *http.Response
+	var err error
+	retries := c.retries
+	c.attempts = 0
+	for {
+		c.attempts++
+		resp, err = c.Client.Do(req)
+		if err != nil {
+			return nil, err //http client errors, not api responses
+		}
 
-	err = CheckResponseError(resp)
-	if err != nil {
-		return nil, err
+		respErr := CheckResponseError(resp)
+		if respErr == nil {
+			break // no errors, break out of the retry loop
+		}
+
+		// retry scenario, close resp and any continue will retry
+		resp.Body.Close()
+
+		if retries <= 1 {
+			return nil, respErr
+		}
+
+		if rateLimitErr, isRetryErr := respErr.(RateLimitError); isRetryErr {
+			// back off and retry
+			wait := time.Duration(rateLimitErr.RetryAfter) * time.Second
+			time.Sleep(wait)
+			retries--
+			continue
+		}
+
+		var doRetry bool
+		switch resp.StatusCode {
+		case http.StatusServiceUnavailable:
+			doRetry = true
+			retries--
+		}
+
+		if doRetry {
+			continue
+		}
+
+		// no retry attempts, just return the err
+		return nil, respErr
 	}
 
+	defer resp.Body.Close()
+
 	if c.apiVersion == defaultApiVersion && resp.Header.Get("X-Shopify-API-Version") != "" {
 		// if using stable on first request set the api version
 		c.apiVersion = resp.Header.Get("X-Shopify-API-Version")
@@ -343,21 +369,30 @@ func (c *Client) doGetHeaders(req *http.Request, v interface{}) (http.Header, er
 }
 
 func wrapSpecificError(r *http.Response, err ResponseError) error {
-	if err.Status == 429 {
-		f, _ := strconv.ParseFloat(r.Header.Get("retry-after"), 64)
+	// see https://www.shopify.dev/concepts/about-apis/response-codes
+	if err.Status == http.StatusTooManyRequests {
+		f, _ := strconv.ParseFloat(r.Header.Get("Retry-After"), 64)
 		return RateLimitError{
 			ResponseError: err,
 			RetryAfter:    int(f),
 		}
 	}
-	if err.Status == 406 {
-		err.Message = "Not acceptable"
+
+	// if err.Status == http.StatusSeeOther {
+	// todo
+	// The response to the request can be found under a different URL in the
+	// Location header and can be retrieved using a GET method on that resource.
+	// }
+
+	if err.Status == http.StatusNotAcceptable {
+		err.Message = http.StatusText(err.Status)
 	}
+
 	return err
 }
 
 func CheckResponseError(r *http.Response) error {
-	if r.StatusCode >= 200 && r.StatusCode < 300 {
+	if http.StatusOK <= r.StatusCode && r.StatusCode < http.StatusMultipleChoices {
 		return nil
 	}
 

goshopify_test.go

@@ -19,6 +19,7 @@ import (
 
 const (
 	testApiVersion = "9999-99"
+	maxRetries     = 3
 )
 
 var (
@@ -45,7 +46,7 @@ func setup() {
 		Scope:       "read_products",
 		Password:    "privateapppassword",
 	}
-	client = NewClient(app, "fooshop", "abcd", WithVersion(testApiVersion))
+	client = NewClient(app, "fooshop", "abcd", WithVersion(testApiVersion), WithRetry(maxRetries))
 	httpmock.ActivateNonDefault(client.Client)
 }
 
@@ -61,38 +62,6 @@ func loadFixture(filename string) []byte {
 	return f
 }
 
-func TestWithVersion(t *testing.T) {
-	c := NewClient(app, "fooshop", "abcd", WithVersion(testApiVersion))
-	expected := fmt.Sprintf("admin/api/%s", testApiVersion)
-	if c.pathPrefix != expected {
-		t.Errorf("WithVersion client.pathPrefix = %s, expected %s", c.pathPrefix, expected)
-	}
-}
-
-func TestWithVersionNoVersion(t *testing.T) {
-	c := NewClient(app, "fooshop", "abcd", WithVersion(""))
-	expected := "admin"
-	if c.pathPrefix != expected {
-		t.Errorf("WithVersion client.pathPrefix = %s, expected %s", c.pathPrefix, expected)
-	}
-}
-
-func TestWithoutVersionInInitiation(t *testing.T) {
-	c := NewClient(app, "fooshop", "abcd")
-	expected := "admin"
-	if c.pathPrefix != expected {
-		t.Errorf("WithVersion client.pathPrefix = %s, expected %s", c.pathPrefix, expected)
-	}
-}
-
-func TestWithVersionInvalidVersion(t *testing.T) {
-	c := NewClient(app, "fooshop", "abcd", WithVersion("9999-99b"))
-	expected := "admin"
-	if c.pathPrefix != expected {
-		t.Errorf("WithVersion client.pathPrefix = %s, expected %s", c.pathPrefix, expected)
-	}
-}
-
 func TestNewClient(t *testing.T) {
 	testClient := NewClient(app, "fooshop", "abcd", WithVersion(testApiVersion))
 	expected := "https://fooshop.myshopify.com"
@@ -338,7 +307,7 @@ func TestDo(t *testing.T) {
 			httpmock.NewStringResponder(406, ``),
 			ResponseError{
 				Status:  406,
-				Message: "Not acceptable",
+				Message: "Not Acceptable",
 			},
 		},
 		{
@@ -379,6 +348,118 @@ func TestDo(t *testing.T) {
 	}
 }
 
+func TestRetry(t *testing.T) {
+	setup()
+	defer teardown()
+
+	type MyStruct struct {
+		Foo string `json:"foo"`
+	}
+
+	var retries int
+	urlFormat := "https://fooshop.myshopify.com/%s"
+
+	cases := []struct {
+		relPath   string
+		responder httpmock.Responder
+		expected  interface{}
+		retries   int
+	}{
+		{ // no retries
+			relPath:  "foo/1",
+			retries:  1,
+			expected: &MyStruct{Foo: "bar"},
+			responder: func(req *http.Request) (*http.Response, error) {
+				return httpmock.NewStringResponse(http.StatusOK, `{"foo": "bar"}`), nil
+			},
+		},
+		{ // 2 retries rate limited, 3 succeeds
+			relPath:  "foo/2",
+			retries:  maxRetries,
+			expected: &MyStruct{Foo: "bar"},
+			responder: func(req *http.Request) (*http.Response, error) {
+				if retries > 1 {
+					resp := httpmock.NewStringResponse(http.StatusTooManyRequests, `{"errors":"Exceeded 2 calls per second for api client. Reduce request rates to resume uninterrupted service."}`)
+					resp.Header.Add("Retry-After", "2.0")
+					retries--
+					return resp, nil
+				}
+
+				return httpmock.NewStringResponse(http.StatusOK, `{"foo": "bar"}`), nil
+			},
+		},
+		{ // all retries rate limited
+			relPath: "foo/3",
+			retries: maxRetries,
+			expected: RateLimitError{
+				RetryAfter: 2,
+				ResponseError: ResponseError{
+					Status:  429,
+					Message: "Exceeded 2 calls per second for api client. Reduce request rates to resume uninterrupted service.",
+				},
+			},
+			responder: func(req *http.Request) (*http.Response, error) {
+				resp := httpmock.NewStringResponse(http.StatusTooManyRequests, `{"errors":"Exceeded 2 calls per second for api client. Reduce request rates to resume uninterrupted service."}`)
+				resp.Header.Add("Retry-After", "2.0")
+				return resp, nil
+			},
+		},
+		{ // 2 retries 503, 3 succeeds
+			relPath:  "foo/4",
+			retries:  maxRetries,
+			expected: &MyStruct{Foo: "bar"},
+			responder: func(req *http.Request) (*http.Response, error) {
+				if retries > 1 {
+					retries--
+					return httpmock.NewStringResponse(http.StatusServiceUnavailable, "<html></html>"), nil
+				}
+
+				return httpmock.NewStringResponse(http.StatusOK, `{"foo": "bar"}`), nil
+			},
+		},
+		{ // all retries 503
+			relPath: "foo/5",
+			retries: maxRetries,
+			expected: ResponseError{
+				Status: http.StatusServiceUnavailable,
+			},
+			responder: func(req *http.Request) (*http.Response, error) {
+				return httpmock.NewStringResponse(http.StatusServiceUnavailable, ""), nil
+			},
+		},
+	}
+
+	for _, c := range cases {
+		retries = c.retries
+		httpmock.RegisterResponder("GET", fmt.Sprintf(urlFormat, c.relPath), c.responder)
+		body := new(MyStruct)
+		req, err := client.NewRequest("GET", c.relPath, nil, nil)
+		if err != nil {
+			t.Error("error creating request: ", err)
+		}
+
+		err = client.Do(req, body)
+
+		if client.attempts != c.retries {
+			t.Errorf("Do(): attempts do not match retries %#v, actual %#v", client.attempts, c.retries)
+		}
+
+		if err != nil {
+			if e, ok := err.(*url.Error); ok {
+				err = e.Err
+			} else if e, ok := err.(*json.SyntaxError); ok {
+				err = errors.New(e.Error())
+			}
+
+			if !reflect.DeepEqual(err, c.expected) {
+				t.Errorf("Do(): expected error %#v, actual %#v", c.expected, err)
+			}
+		} else if err == nil && !reflect.DeepEqual(body, c.expected) {
+			t.Errorf("Do(): expected %#v, actual %#v", c.expected, body)
+		}
+	}
+}
+
 func TestClientDoAutoApiVersion(t *testing.T) {
 	u := "foo/1"
 	responder := func(req *http.Request) (*http.Response, error) {

options.go

@@ -0,0 +1,24 @@
+package goshopify
+
+import "fmt"
+
+// Option is used to configure client with options
+type Option func(c *Client)
+
+// WithVersion optionally sets the api-version if the passed string is valid
+func WithVersion(apiVersion string) Option {
+	return func(c *Client) {
+		pathPrefix := defaultApiPathPrefix
+		if len(apiVersion) > 0 && apiVersionRegex.MatchString(apiVersion) {
+			pathPrefix = fmt.Sprintf("admin/api/%s", apiVersion)
+		}
+		c.apiVersion = apiVersion
+		c.pathPrefix = pathPrefix
+	}
+}
+
+func WithRetry(retries int) Option {
+	return func(c *Client) {
+		c.retries = retries
+	}
+}

options_test.go

@@ -0,0 +1,46 @@
+package goshopify
+
+import (
+	"fmt"
+	"testing"
+)
+
+func TestWithVersion(t *testing.T) {
+	c := NewClient(app, "fooshop", "abcd", WithVersion(testApiVersion))
+	expected := fmt.Sprintf("admin/api/%s", testApiVersion)
+	if c.pathPrefix != expected {
+		t.Errorf("WithVersion client.pathPrefix = %s, expected %s", c.pathPrefix, expected)
+	}
+}
+
+func TestWithVersionNoVersion(t *testing.T) {
+	c := NewClient(app, "fooshop", "abcd", WithVersion(""))
+	expected := "admin"
+	if c.pathPrefix != expected {
+		t.Errorf("WithVersion client.pathPrefix = %s, expected %s", c.pathPrefix, expected)
+	}
+}
+
+func TestWithoutVersionInInitiation(t *testing.T) {
+	c := NewClient(app, "fooshop", "abcd")
+	expected := "admin"
+	if c.pathPrefix != expected {
+		t.Errorf("WithVersion client.pathPrefix = %s, expected %s", c.pathPrefix, expected)
+	}
+}
+
+func TestWithVersionInvalidVersion(t *testing.T) {
+	c := NewClient(app, "fooshop", "abcd", WithVersion("9999-99b"))
+	expected := "admin"
+	if c.pathPrefix != expected {
+		t.Errorf("WithVersion client.pathPrefix = %s, expected %s", c.pathPrefix, expected)
+	}
+}
+
+func TestWithRetry(t *testing.T) {
+	c := NewClient(app, "fooshop", "abcd", WithRetry(5))
+	expected := 5
+	if c.retries != expected {
+		t.Errorf("WithRetry client.retries = %d, expected %d", c.retries, expected)
+	}
+}