Merge pull request #277 from flimzy/decode429Errors

Properly decode 429 error responses

category_test.go

@@ -3,7 +3,6 @@ package spotify
 import (
 	"context"
 	"net/http"
-	"strings"
 	"testing"
 )
 
@@ -93,8 +92,8 @@ func TestGetCategoryPlaylistsOpt(t *testing.T) {
 	defer server.Close()
 
 	_, err := client.GetCategoryPlaylists(context.Background(), "id", Limit(5), Offset(10))
-	if err == nil || !strings.Contains(err.Error(), "HTTP 404: Not Found") {
-		t.Errorf("Expected error 'spotify: HTTP 404: Not Found (body empty)', got %v", err)
+	if want := "Not Found"; err == nil || err.Error() != want {
+		t.Errorf("Expected error: want %v, got %v", want, err)
 	}
 }
 

playlist_test.go

@@ -7,7 +7,6 @@ import (
 	"fmt"
 	"io"
 	"net/http"
-	"strings"
 	"testing"
 	"time"
 )
@@ -606,7 +605,7 @@ func TestClient_ReplacePlaylistItems(t *testing.T) {
 				items:      []URI{"spotify:track:track1", "spotify:track:track2"},
 			},
 			want: want{
-				err: "spotify: HTTP 403: Forbidden (body empty)",
+				err: "Forbidden",
 			},
 		},
 	}
@@ -714,8 +713,8 @@ func TestReorderPlaylistRequest(t *testing.T) {
 		RangeStart:   3,
 		InsertBefore: 8,
 	})
-	if err == nil || !strings.Contains(err.Error(), "HTTP 404: Not Found") {
-		t.Errorf("Expected error 'spotify: HTTP 404: Not Found (body empty)', got %v", err)
+	if want := "Not Found"; err == nil || err.Error() != want {
+		t.Errorf("Expected error: want %v, got %v", want, err)
 	}
 }
 

spotify.go

@@ -30,10 +30,6 @@ const (
 	// defaultRetryDurationS helps us fix an apparent server bug whereby we will
 	// be told to retry but not be given a wait-interval.
 	defaultRetryDuration = time.Second * 5
-
-	// rateLimitExceededStatusCode is the code that the server returns when our
-	// request frequency is too high.
-	rateLimitExceededStatusCode = 429
 )
 
 // Client is a client for working with the Spotify Web API.
@@ -159,11 +155,22 @@ func (e Error) Error() string {
 }
 
 // decodeError decodes an Error from an io.Reader.
-func (c *Client) decodeError(resp *http.Response) error {
+func decodeError(resp *http.Response) error {
 	responseBody, err := io.ReadAll(resp.Body)
 	if err != nil {
 		return err
 	}
+	if ctHeader := resp.Header.Get("Content-Type"); ctHeader == "" {
+		msg := string(responseBody)
+		if len(msg) == 0 {
+			msg = http.StatusText(resp.StatusCode)
+		}
+
+		return Error{
+			Message: msg,
+			Status:  resp.StatusCode,
+		}
+	}
 
 	if len(responseBody) == 0 {
 		return fmt.Errorf("spotify: HTTP %d: %s (body empty)", resp.StatusCode, http.StatusText(resp.StatusCode))
@@ -239,7 +246,7 @@ func (c *Client) execute(req *http.Request, result interface{}, needsStatus ...i
 		if (resp.StatusCode >= 300 ||
 			resp.StatusCode < 200) &&
 			isFailure(resp.StatusCode, needsStatus) {
-			return c.decodeError(resp)
+			return decodeError(resp)
 		}
 
 		if result != nil {
@@ -280,7 +287,7 @@ func (c *Client) get(ctx context.Context, url string, result interface{}) error
 
 		defer resp.Body.Close()
 
-		if resp.StatusCode == rateLimitExceededStatusCode && c.autoRetry {
+		if resp.StatusCode == http.StatusTooManyRequests && c.autoRetry {
 			select {
 			case <-ctx.Done():
 				// If the context is cancelled, return the original error
@@ -292,7 +299,7 @@ func (c *Client) get(ctx context.Context, url string, result interface{}) error
 			return nil
 		}
 		if resp.StatusCode != http.StatusOK {
-			return c.decodeError(resp)
+			return decodeError(resp)
 		}
 
 		return json.NewDecoder(resp.Body).Decode(result)

spotify_test.go

@@ -2,14 +2,15 @@ package spotify
 
 import (
 	"context"
-	"golang.org/x/oauth2"
 	"io"
 	"net/http"
 	"net/http/httptest"
 	"os"
 	"strings"
 	"testing"
 	"time"
+
+	"golang.org/x/oauth2"
 )
 
 func testClient(code int, body io.Reader, validators ...func(*http.Request)) (*Client, *httptest.Server) {
@@ -70,7 +71,7 @@ func TestNewReleasesRateLimitExceeded(t *testing.T) {
 		// first attempt fails
 		http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 			w.Header().Set("Retry-After", "2")
-			w.WriteHeader(rateLimitExceededStatusCode)
+			w.WriteHeader(http.StatusTooManyRequests)
 			_, _ = io.WriteString(w, `{ "error": { "message": "slow down", "status": 429 } }`)
 		}),
 		// next attempt succeeds
@@ -144,14 +145,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)
@@ -161,3 +162,19 @@ func TestClient_Token(t *testing.T) {
 		}
 	})
 }
+
+func TestDecode429Error(t *testing.T) {
+	resp := &http.Response{
+		StatusCode: http.StatusTooManyRequests,
+		Header:     http.Header{"Retry-After": []string{"2"}},
+		Body:       io.NopCloser(strings.NewReader(`Too many requests`)),
+	}
+
+	err := decodeError(resp)
+	if err == nil {
+		t.Fatal("Expected error")
+	}
+	if err.Error() != "Too many requests" {
+		t.Error("Invalid error message:", err.Error())
+	}
+}

user_test.go

@@ -129,7 +129,7 @@ func TestFollowArtistAutoRetry(t *testing.T) {
 		// first attempt fails
 		http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 			w.Header().Set("Retry-After", "2")
-			w.WriteHeader(rateLimitExceededStatusCode)
+			w.WriteHeader(http.StatusTooManyRequests)
 			_, _ = io.WriteString(w, `{ "error": { "message": "slow down", "status": 429 } }`)
 		}),
 		// next attempt succeeds