From 4a20a3556cdc37fd385ac91fa1adc5c3520b2486 Mon Sep 17 00:00:00 2001 From: Jules Rosier Date: Thu, 14 Nov 2024 15:56:53 +0100 Subject: [PATCH 1/7] added maxRetryDuration option to client --- spotify.go | 30 ++++++++++++++++++++++++++---- spotify_test.go | 30 +++++++++++++++++++++++++++--- 2 files changed, 53 insertions(+), 7 deletions(-) diff --git a/spotify.go b/spotify.go index 63851d6..486e6cd 100644 --- a/spotify.go +++ b/spotify.go @@ -40,14 +40,19 @@ const ( rateLimitExceededStatusCode = 429 ) +// ErrNoMorePages is the error returned when response header has a 'Retry-After' +// duration longer then the configuerd max. +var ErrMaxRetryDurationExceeded = errors.New("spotify: retry would take longer then configured max") + // Client is a client for working with the Spotify Web API. // It is best to create this using spotify.New() type Client struct { http *http.Client baseURL string - autoRetry bool - acceptLanguage string + autoRetry bool + acceptLanguage string + maxRetryDuration time.Duration } type ClientOption func(client *Client) @@ -74,6 +79,15 @@ func WithAcceptLanguage(lang string) ClientOption { } } +// WithMaxRetryDuration limits the amount of time that the client wil wait to retry afther being rate limited. +// If the retry time is longer then the max, then the client will return an err +// This option only works when auto retry is enabled +func WithMaxRetryDuration(duration time.Duration) ClientOption { + return func(client *Client) { + client.maxRetryDuration = duration + } +} + // New returns a client for working with the Spotify Web API. // The provided httpClient must provide Authentication with the requests. // The auth package may be used to generate a suitable client. @@ -230,10 +244,14 @@ func (c *Client) execute(req *http.Request, result interface{}, needsStatus ...i if c.autoRetry && isFailure(resp.StatusCode, needsStatus) && shouldRetry(resp.StatusCode) { + duration := retryDuration(resp) + if c.maxRetryDuration > 0 && duration > c.maxRetryDuration { + return ErrMaxRetryDurationExceeded + } select { case <-req.Context().Done(): // If the context is cancelled, return the original error - case <-time.After(retryDuration(resp)): + case <-time.After(duration): continue } } @@ -285,10 +303,14 @@ func (c *Client) get(ctx context.Context, url string, result interface{}) error defer resp.Body.Close() if resp.StatusCode == rateLimitExceededStatusCode && c.autoRetry { + duration := retryDuration(resp) + if c.maxRetryDuration > 0 && duration > c.maxRetryDuration { + return ErrMaxRetryDurationExceeded + } select { case <-ctx.Done(): // If the context is cancelled, return the original error - case <-time.After(retryDuration(resp)): + case <-time.After(duration): continue } } diff --git a/spotify_test.go b/spotify_test.go index 9eb11e0..5ed2a02 100644 --- a/spotify_test.go +++ b/spotify_test.go @@ -2,7 +2,6 @@ package spotify import ( "context" - "golang.org/x/oauth2" "io" "net/http" "net/http/httptest" @@ -10,6 +9,8 @@ import ( "strings" "testing" "time" + + "golang.org/x/oauth2" ) func testClient(code int, body io.Reader, validators ...func(*http.Request)) (*Client, *httptest.Server) { @@ -104,6 +105,29 @@ func TestNewReleasesRateLimitExceeded(t *testing.T) { } } +func TestNewReleasesMaxRetry(t *testing.T) { + t.Parallel() + handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Retry-After", "3660") // 61 minutes + w.WriteHeader(rateLimitExceededStatusCode) + _, _ = io.WriteString(w, `{ "error": { "message": "slow down", "status": 429 } }`) + }) + + server := httptest.NewServer(handler) + defer server.Close() + + client := &Client{ + http: http.DefaultClient, + baseURL: server.URL + "/", + autoRetry: true, + maxRetryDuration: time.Hour, + } + _, err := client.NewReleases(context.Background()) + if err != ErrMaxRetryDurationExceeded { + t.Errorf("Should throw 'ErrMaxRetryDurationExceeded' error, got '%s'", err) + } +} + func TestClient_Token(t *testing.T) { // oauth setup for valid test token config := oauth2.Config{ @@ -144,14 +168,14 @@ func TestClient_Token(t *testing.T) { t.Run("non oauth2 transport", func(t *testing.T) { client := &Client{ - http: http.DefaultClient, + http: http.DefaultClient, } _, err := client.Token() if err == nil || err.Error() != "spotify: client not backed by oauth2 transport" { t.Errorf("Should throw error: %s", "spotify: client not backed by oauth2 transport") } }) - + t.Run("invalid token", func(t *testing.T) { httpClient := config.Client(context.Background(), nil) client := New(httpClient) From 4ac77dff5f0cec769809728e551b99b973485089 Mon Sep 17 00:00:00 2001 From: Jules <55190371+Pineapple217@users.noreply.github.com> Date: Thu, 14 Nov 2024 21:00:52 +0100 Subject: [PATCH 2/7] fix spelling Co-authored-by: Jonathan Hall --- spotify.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spotify.go b/spotify.go index 486e6cd..b8e5504 100644 --- a/spotify.go +++ b/spotify.go @@ -42,7 +42,7 @@ const ( // ErrNoMorePages is the error returned when response header has a 'Retry-After' // duration longer then the configuerd max. -var ErrMaxRetryDurationExceeded = errors.New("spotify: retry would take longer then configured max") +var ErrMaxRetryDurationExceeded = errors.New("spotify: retry would take longer than configured max") // Client is a client for working with the Spotify Web API. // It is best to create this using spotify.New() From 41b1796e708e45dbf35998f10c2c1dc7265003c9 Mon Sep 17 00:00:00 2001 From: Jules <55190371+Pineapple217@users.noreply.github.com> Date: Thu, 14 Nov 2024 21:01:30 +0100 Subject: [PATCH 3/7] fix spelling Co-authored-by: Jonathan Hall --- spotify.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spotify.go b/spotify.go index b8e5504..2d75dd5 100644 --- a/spotify.go +++ b/spotify.go @@ -80,7 +80,7 @@ func WithAcceptLanguage(lang string) ClientOption { } // WithMaxRetryDuration limits the amount of time that the client wil wait to retry afther being rate limited. -// If the retry time is longer then the max, then the client will return an err +// If the retry time is longer than the max, then the client will return an error. // This option only works when auto retry is enabled func WithMaxRetryDuration(duration time.Duration) ClientOption { return func(client *Client) { From ccb251b1fd8d56d383fa801a30c8024aa52dd281 Mon Sep 17 00:00:00 2001 From: Jules <55190371+Pineapple217@users.noreply.github.com> Date: Thu, 14 Nov 2024 21:01:56 +0100 Subject: [PATCH 4/7] fix spelling Co-authored-by: Jonathan Hall --- spotify.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spotify.go b/spotify.go index 2d75dd5..a5faafc 100644 --- a/spotify.go +++ b/spotify.go @@ -79,7 +79,7 @@ func WithAcceptLanguage(lang string) ClientOption { } } -// WithMaxRetryDuration limits the amount of time that the client wil wait to retry afther being rate limited. +// WithMaxRetryDuration limits the amount of time that the client will wait to retry after being rate limited. // If the retry time is longer than the max, then the client will return an error. // This option only works when auto retry is enabled func WithMaxRetryDuration(duration time.Duration) ClientOption { From 9ad9b52cafe77477bfb2c792f2caed5ee6f571c7 Mon Sep 17 00:00:00 2001 From: Jules <55190371+Pineapple217@users.noreply.github.com> Date: Thu, 14 Nov 2024 22:18:43 +0100 Subject: [PATCH 5/7] fix spelling Co-authored-by: Jonathan Hall --- spotify.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spotify.go b/spotify.go index a5faafc..cdcf9e8 100644 --- a/spotify.go +++ b/spotify.go @@ -41,7 +41,7 @@ const ( ) // ErrNoMorePages is the error returned when response header has a 'Retry-After' -// duration longer then the configuerd max. +// duration longer then the configured max. var ErrMaxRetryDurationExceeded = errors.New("spotify: retry would take longer than configured max") // Client is a client for working with the Spotify Web API. From caf1160097889357226917bc909a42199ff82710 Mon Sep 17 00:00:00 2001 From: Jules Rosier Date: Fri, 15 Nov 2024 17:29:49 +0100 Subject: [PATCH 6/7] Custom err type for maxRetryDuration --- spotify.go | 14 ++++++++++---- spotify_test.go | 9 +++++++-- 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/spotify.go b/spotify.go index cdcf9e8..d2f310b 100644 --- a/spotify.go +++ b/spotify.go @@ -40,9 +40,15 @@ const ( rateLimitExceededStatusCode = 429 ) -// ErrNoMorePages is the error returned when response header has a 'Retry-After' +// MaxRetryDurationExceededErr is the error type returned when response header has a 'Retry-After' // duration longer then the configured max. -var ErrMaxRetryDurationExceeded = errors.New("spotify: retry would take longer than configured max") +type MaxRetryDurationExceededErr struct { + RetryAfter time.Duration +} + +func (e *MaxRetryDurationExceededErr) Error() string { + return "spotify: retry would take longer than configured max" +} // Client is a client for working with the Spotify Web API. // It is best to create this using spotify.New() @@ -246,7 +252,7 @@ func (c *Client) execute(req *http.Request, result interface{}, needsStatus ...i shouldRetry(resp.StatusCode) { duration := retryDuration(resp) if c.maxRetryDuration > 0 && duration > c.maxRetryDuration { - return ErrMaxRetryDurationExceeded + return &MaxRetryDurationExceededErr{RetryAfter: duration} } select { case <-req.Context().Done(): @@ -305,7 +311,7 @@ func (c *Client) get(ctx context.Context, url string, result interface{}) error if resp.StatusCode == rateLimitExceededStatusCode && c.autoRetry { duration := retryDuration(resp) if c.maxRetryDuration > 0 && duration > c.maxRetryDuration { - return ErrMaxRetryDurationExceeded + return &MaxRetryDurationExceededErr{RetryAfter: duration} } select { case <-ctx.Done(): diff --git a/spotify_test.go b/spotify_test.go index 5ed2a02..8a6b82c 100644 --- a/spotify_test.go +++ b/spotify_test.go @@ -2,6 +2,7 @@ package spotify import ( "context" + "errors" "io" "net/http" "net/http/httptest" @@ -123,8 +124,12 @@ func TestNewReleasesMaxRetry(t *testing.T) { maxRetryDuration: time.Hour, } _, err := client.NewReleases(context.Background()) - if err != ErrMaxRetryDurationExceeded { - t.Errorf("Should throw 'ErrMaxRetryDurationExceeded' error, got '%s'", err) + var maxErr *MaxRetryDurationExceededErr + if !errors.As(err, &maxErr) { + t.Errorf("Should throw 'MaxRetryDurationExceededErr' type error, got '%s'", err) + } + if maxErr.RetryAfter != time.Second*3660 { + t.Errorf("Error had wrong 'RetryAfter' value, got %f", maxErr.RetryAfter.Seconds()) } } From c8fd613ffd92f313b99c335b6679e27cf7f9b9ad Mon Sep 17 00:00:00 2001 From: Jules Rosier Date: Fri, 29 Nov 2024 17:10:01 +0100 Subject: [PATCH 7/7] Added back max retry test --- spotify_test.go | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/spotify_test.go b/spotify_test.go index 9154cb6..8747d42 100644 --- a/spotify_test.go +++ b/spotify_test.go @@ -153,6 +153,33 @@ func TestRateLimitExceededReportsRetryAfter(t *testing.T) { } } +func TestRateLimitExceededMaxRetryConfig(t *testing.T) { + t.Parallel() + const retryAfter = 3660 // 61 minutes + handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Retry-After", strconv.Itoa(retryAfter)) + w.WriteHeader(http.StatusTooManyRequests) + _, _ = io.WriteString(w, `{ "error": { "message": "slow down", "status": 429 } }`) + }) + server := httptest.NewServer(handler) + defer server.Close() + client := &Client{ + http: http.DefaultClient, + baseURL: server.URL + "/", + autoRetry: true, + maxRetryDuration: time.Hour, + } + + _, err := client.NewReleases(context.Background()) + var spotifyError Error + if !errors.As(err, &spotifyError) { + t.Fatalf("expected a spotify error, got %T", err) + } + if retryAfter*time.Second-time.Until(spotifyError.RetryAfter) > time.Second { + t.Error("expected RetryAfter value") + } +} + func TestClient_Token(t *testing.T) { // oauth setup for valid test token config := oauth2.Config{