Back off and retry whenever API rate limits are reached (#10)

Atlassian applies rate limiting to Jira's REST APIs [1]. When the rate
limit for a particular endpoint has been reached, Jira produces a
response that is compatible with RFC 6585 section 4: the status is set
to `429 Too Many Requests`, and the client is expected not to reattempt
the request until at least the number of seconds given by the
`Retry-After` header have elapsed. Currently the default `Client` and
the built-in `Transport`s do not respect any rate limits, and exceeding
them results in errors.

Use `github.com/hashicorp/go-retryablehttp` to implement RFC 6585
section 4 in the default `Client` and built-in `Transport`s so that the
API rate limits are respected. A welcome side-effect is that failed
requests due to (e.g.) network problems will also be retried (up to a
maximum of 4 times, go-retryablehttp's default).

[1] https://developer.atlassian.com/cloud/jira/platform/rate-limiting/

go.mod

@@ -7,6 +7,7 @@ require (
 	github.com/golang-jwt/jwt/v4 v4.4.2
 	github.com/google/go-cmp v0.5.8
 	github.com/google/go-querystring v1.1.0
+	github.com/hashicorp/go-retryablehttp v0.7.5
 	github.com/pkg/errors v0.9.1
 	github.com/trivago/tgo v1.0.7
 	golang.org/x/sys v0.0.0-20220330033206-e17cdc41300f // indirect

go.sum

@@ -1,3 +1,5 @@
+github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c=
+github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
 github.com/fatih/structs v1.1.0 h1:Q7juDM0QtcnhCpeyLGQKyg4TOIghuNXrkL32pHAUMxo=
 github.com/fatih/structs v1.1.0/go.mod h1:9NiDSp5zOcgEDl+j00MP/WkGVPOlPRLejGD8Ga6PJ7M=
 github.com/golang-jwt/jwt/v4 v4.4.2 h1:rcc4lwaZgFMCZ5jxF9ABolDcIHdBytAFgqFPbSJQAYs=
@@ -7,8 +9,18 @@ github.com/google/go-cmp v0.5.8 h1:e6P7q2lk1O+qJJb4BtCQXlK8vWEO8V1ZeuEdJNOqZyg=
 github.com/google/go-cmp v0.5.8/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY=
 github.com/google/go-querystring v1.1.0 h1:AnCroh3fv4ZBgVIf1Iwtovgjaw/GiKJo8M8yD/fhyJ8=
 github.com/google/go-querystring v1.1.0/go.mod h1:Kcdr2DB4koayq7X8pmAG4sNG59So17icRSOU623lUBU=
+github.com/hashicorp/go-cleanhttp v0.5.2 h1:035FKYIWjmULyFRBKPs8TBQoi0x6d9G4xc9neXJWAZQ=
+github.com/hashicorp/go-cleanhttp v0.5.2/go.mod h1:kO/YDlP8L1346E6Sodw+PrpBSV4/SoxCXGY6BqNFT48=
+github.com/hashicorp/go-hclog v0.9.2 h1:CG6TE5H9/JXsFWJCfoIVpKFIkFe6ysEuHirp4DxCsHI=
+github.com/hashicorp/go-hclog v0.9.2/go.mod h1:5CU+agLiy3J7N7QjHK5d05KxGsuXiQLrjA0H7acj2lQ=
+github.com/hashicorp/go-retryablehttp v0.7.5 h1:bJj+Pj19UZMIweq/iie+1u5YCdGrnxCT9yvm0e+Nd5M=
+github.com/hashicorp/go-retryablehttp v0.7.5/go.mod h1:Jy/gPYAdjqffZ/yFGCFV2doI5wjtH1ewM9u8iYVjtX8=
 github.com/pkg/errors v0.9.1 h1:FEBLx1zS214owpjy7qsBeixbURkuhQAwrK5UwLGTwt4=
 github.com/pkg/errors v0.9.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0=
+github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
+github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
+github.com/stretchr/testify v1.2.2 h1:bSDNvY7ZPG5RlJ8otE/7V6gMiyenm9RtJ7IUVIAoJ1w=
+github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs=
 github.com/trivago/tgo v1.0.7 h1:uaWH/XIy9aWYWpjm2CU3RpcqZXmX2ysQ9/Go+d9gyrM=
 github.com/trivago/tgo v1.0.7/go.mod h1:w4dpD+3tzNIIiIfkWWa85w5/B77tlvdZckQ+6PkFnhc=
 golang.org/x/sys v0.0.0-20201119102817-f84b799fce68/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=

jira.go

@@ -17,6 +17,7 @@ import (
 
 	jwt "github.com/golang-jwt/jwt/v4"
 	"github.com/google/go-querystring/query"
+	"github.com/hashicorp/go-retryablehttp"
 	"github.com/pkg/errors"
 )
 
@@ -62,16 +63,32 @@ type Client struct {
 	Request          *RequestService
 }
 
+// defaultClient is the underlying HTTP client used by the Jira API client if no other HTTP client
+// is specified.
+//
+// Both the Jira REST API and retryablehttp.DefaultRetryPolicy implement RFC 6585 section 4, in
+// which the server responds with status code 429 if too many requests have been sent and the
+// client backs off for at least the number of seconds given in the Retry-After response header
+// before retrying; this means that a retryablehttp.Client with default settings is all that is
+// needed to obey Jira's API rate limits.
+var defaultClient = retryablehttp.NewClient()
+
+// defaultTransport is the underlying Transport used by the other Transports in this package if no
+// other Transport is specified. It ensures that failed requests are automatically retried.
+var defaultTransport = &retryablehttp.RoundTripper{Client: defaultClient}
+
 // NewClient returns a new Jira API client.
-// If a nil httpClient is provided, http.DefaultClient will be used.
+// If a nil httpClient is provided, a retryablehttp.Client with default settings will be used; this
+// ensures that requests that exceed Jira's rate limits are automatically retried after the cooldown
+// period.
 // To use API methods which require authentication you can follow the preferred solution and
 // provide an http.Client that will perform the authentication for you with OAuth and HTTP Basic (such as that provided by the golang.org/x/oauth2 library).
 // As an alternative you can use Session Cookie based authentication provided by this package as well.
 // See https://docs.atlassian.com/jira/REST/latest/#authentication
 // baseURL is the HTTP endpoint of your Jira instance and should always be specified with a trailing slash.
 func NewClient(httpClient httpClient, baseURL string) (*Client, error) {
 	if httpClient == nil {
-		httpClient = http.DefaultClient
+		httpClient = defaultClient.StandardClient()
 	}
 
 	// ensure the baseURL contains a trailing slash so that all paths are preserved in later calls
@@ -357,7 +374,7 @@ type BasicAuthTransport struct {
 	Password string
 
 	// Transport is the underlying HTTP transport to use when making requests.
-	// It will default to http.DefaultTransport if nil.
+	// If nil, defaults to a Transport that automatically retries the request on failure.
 	Transport http.RoundTripper
 }
 
@@ -383,7 +400,7 @@ func (t *BasicAuthTransport) transport() http.RoundTripper {
 	if t.Transport != nil {
 		return t.Transport
 	}
-	return http.DefaultTransport
+	return defaultTransport
 }
 
 // BearerAuthTransport is a http.RoundTripper that authenticates all requests
@@ -392,7 +409,7 @@ type BearerAuthTransport struct {
 	Token string
 
 	// Transport is the underlying HTTP transport to use when making requests.
-	// It will default to http.DefaultTransport if nil.
+	// If nil, defaults to a Transport that automatically retries the request on failure.
 	Transport http.RoundTripper
 }
 
@@ -418,7 +435,7 @@ func (t *BearerAuthTransport) transport() http.RoundTripper {
 	if t.Transport != nil {
 		return t.Transport
 	}
-	return http.DefaultTransport
+	return defaultTransport
 }
 
 // PATAuthTransport is an http.RoundTripper that authenticates all requests
@@ -429,7 +446,7 @@ type PATAuthTransport struct {
 	Token string
 
 	// Transport is the underlying HTTP transport to use when making requests.
-	// It will default to http.DefaultTransport if nil.
+	// If nil, defaults to a Transport that automatically retries the request on failure.
 	Transport http.RoundTripper
 }
 
@@ -454,7 +471,7 @@ func (t *PATAuthTransport) transport() http.RoundTripper {
 	if t.Transport != nil {
 		return t.Transport
 	}
-	return http.DefaultTransport
+	return defaultTransport
 }
 
 // CookieAuthTransport is an http.RoundTripper that authenticates all requests
@@ -474,7 +491,7 @@ type CookieAuthTransport struct {
 	SessionObject []*http.Cookie
 
 	// Transport is the underlying HTTP transport to use when making requests.
-	// It will default to http.DefaultTransport if nil.
+	// If nil, defaults to a Transport that automatically retries the request on failure.
 	Transport http.RoundTripper
 }
 
@@ -554,7 +571,7 @@ func (t *CookieAuthTransport) transport() http.RoundTripper {
 	if t.Transport != nil {
 		return t.Transport
 	}
-	return http.DefaultTransport
+	return defaultTransport
 }
 
 // JWTAuthTransport is an http.RoundTripper that authenticates all requests
@@ -571,7 +588,7 @@ type JWTAuthTransport struct {
 	Issuer string
 
 	// Transport is the underlying HTTP transport to use when making requests.
-	// It will default to http.DefaultTransport if nil.
+	// If nil, defaults to a Transport that automatically retries the request on failure.
 	Transport http.RoundTripper
 }
 
@@ -583,7 +600,7 @@ func (t *JWTAuthTransport) transport() http.RoundTripper {
 	if t.Transport != nil {
 		return t.Transport
 	}
-	return http.DefaultTransport
+	return defaultTransport
 }
 
 // RoundTrip adds the session object to the request.

jira_test.go

@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"fmt"
 	"io/ioutil"
+	"math"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
@@ -432,6 +433,42 @@ func TestClient_Do_RedirectLoop(t *testing.T) {
 	}
 }
 
+// Test handling of responses that simulate an API rate limit being exceeded.
+// The client should automatically back off until the duration given by the Retry-After response
+// header has elapsed, meaning that the request should succeed from the caller's perspective.
+func TestClient_Do_TooManyRequests(t *testing.T) {
+	setup()
+	defer teardown()
+
+	raw, err := ioutil.ReadFile("./mocks/too_many_requests.html")
+	if err != nil {
+		t.Error(err.Error())
+	}
+
+	// Permit requests after 3 seconds have elapsed
+	cooloff := time.Now().Add(3 * time.Second)
+
+	testMux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
+		remaining := int(math.Ceil(time.Until(cooloff).Seconds()))
+		if remaining > 0 {
+			w.Header().Set("Retry-After", fmt.Sprintf("%d", remaining))
+			http.Error(w, string(raw), 429)
+		} else {
+			fmt.Fprint(w, `{}`)
+		}
+	})
+
+	req, _ := testClient.NewRequest("GET", "/", nil)
+	res, _ := testClient.Do(req, nil)
+	_, err = ioutil.ReadAll(res.Body)
+
+	if err != nil {
+		t.Errorf("Error on parsing HTTP Response = %v", err.Error())
+	} else if res.StatusCode != 200 {
+		t.Errorf("Response code = %v, want %v", res.StatusCode, 200)
+	}
+}
+
 func TestClient_GetBaseURL_WithURL(t *testing.T) {
 	u, err := url.Parse(testJiraInstanceURL)
 	if err != nil {
@@ -480,18 +517,73 @@ func TestBasicAuthTransport(t *testing.T) {
 	basicAuthClient.Do(req, nil)
 }
 
+// Test handling of responses that simulate an API rate limit being exceeded when the client is
+// backed by BasicAuthTransport.
+// The client should automatically back off until the duration given by the Retry-After response
+// header has elapsed, meaning that the request should succeed from the caller's perspective.
+func TestBasicAuthTransport_TooManyRequests(t *testing.T) {
+	setup()
+	defer teardown()
+
+	username, password := "username", "password"
+
+	raw, err := ioutil.ReadFile("./mocks/too_many_requests.html")
+	if err != nil {
+		t.Error(err.Error())
+	}
+
+	// Permit requests after 3 seconds have elapsed
+	cooloff := time.Now().Add(3 * time.Second)
+
+	testMux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
+		u, p, ok := r.BasicAuth()
+		if !ok {
+			t.Errorf("request does not contain basic auth credentials")
+		}
+		if u != username {
+			t.Errorf("request contained basic auth username %q, want %q", u, username)
+		}
+		if p != password {
+			t.Errorf("request contained basic auth password %q, want %q", p, password)
+		}
+		remaining := int(math.Ceil(time.Until(cooloff).Seconds()))
+		if remaining > 0 {
+			w.Header().Set("Retry-After", fmt.Sprintf("%d", remaining))
+			http.Error(w, string(raw), 429)
+		} else {
+			fmt.Fprint(w, `{}`)
+		}
+	})
+
+	tp := &BasicAuthTransport{
+		Username: username,
+		Password: password,
+	}
+
+	basicAuthClient, _ := NewClient(tp.Client(), testServer.URL)
+	req, _ := basicAuthClient.NewRequest("GET", "/", nil)
+	res, _ := basicAuthClient.Do(req, nil)
+	_, err = ioutil.ReadAll(res.Body)
+
+	if err != nil {
+		t.Errorf("Error on parsing HTTP Response = %v", err.Error())
+	} else if res.StatusCode != 200 {
+		t.Errorf("Response code = %v, want %v", res.StatusCode, 200)
+	}
+}
+
 func TestBasicAuthTransport_transport(t *testing.T) {
 	// default transport
 	tp := &BasicAuthTransport{}
-	if tp.transport() != http.DefaultTransport {
-		t.Errorf("Expected http.DefaultTransport to be used.")
+	if tp.transport() != defaultTransport {
+		t.Errorf("Expected defaultTransport to be used.")
 	}
 
 	// custom transport
 	tp = &BasicAuthTransport{
 		Transport: &http.Transport{},
 	}
-	if tp.transport() == http.DefaultTransport {
+	if tp.transport() == defaultTransport {
 		t.Errorf("Expected custom transport to be used.")
 	}
 }

mocks/too_many_requests.html

@@ -0,0 +1 @@
+<!DOCTYPE html><html lang="en"><head><meta charset="utf-8"><meta http-equiv="X-UA-Compatible" content="IE=edge"><title>Oops - an error has occurred</title><link type='text/css' rel='stylesheet' href='/static-assets/metal-all.css' media='all'><script src='/static-assets/metal-all.js'></script><meta name="decorator" content="none" /></head><body class=" error-page error429"><script type="text/javascript">document.body.className += " js-enabled";</script><div id="page"><header id="header" role="banner"></header><!-- #header --><section id="content" role="main"><div class="aui-page-panel"><div class="aui-page-panel-inner"><section class="aui-page-panel-content lowerContent"><div id="error-state"><span class="error-type"></span><h1>Something went wrong</h1><p>Try reloading the page, then check our <a href="https://status.atlassian.com">Statuspage</a> for any current outages. If there are no relevant outages, create a <a href="https://support.atlassian.com/contact/">support request</a> so we can help you out.</p><p>If you create a request, include the following so we can help you as fast as possible:</p><ul><li>Error type: <strong>429 - Too many requests</strong></li><li>Log reference: <strong>00000000-0000-4000-0000-000000000000</strong></li></ul></div></section><!-- .aui-page-panel-content --></div><!-- .aui-page-panel-inner --></div><!-- .aui-page-panel --></section><!-- #content --><footer id="footer" role="contentinfo"><section class="footer-body"><div id="footer-logo"><a href="http://www.atlassian.com/" rel="nofollow">Atlassian</a></div></section></footer><!-- #footer --></div><!-- #page --></body></html>