Properly set Error.StatusCode

fixes #154 newError was completely ingoring response status code. It was wrongly deriving it from error code. This patch ensures Error,StatusCode is set with correct response status.
ably · Apr 23, 2020 · 8d37982 · 8d37982
1 parent 74d6872
commit 8d37982
Show file tree

Hide file tree

Showing 2 changed files with 94 additions and 6 deletions.
diff --git a/ably/error.go b/ably/error.go
@@ -3,7 +3,6 @@ package ably
 import (
 	"errors"
 	"fmt"
-	"io"
 	"io/ioutil"
 	"mime"
 	"net"
@@ -89,12 +88,12 @@ func code(err error) int {
 	return 0
 }
 
-func errFromUnprocessableBody(r io.Reader) error {
-	errMsg, err := ioutil.ReadAll(r)
+func errFromUnprocessableBody(resp *http.Response) error {
+	errMsg, err := ioutil.ReadAll(resp.Body)
 	if err == nil {
 		err = errors.New(string(errMsg))
 	}
-	return newError(40000, err)
+	return &Error{Code: 40000, StatusCode: resp.StatusCode, Err: err}
 }
 
 func checkValidHTTPResponse(resp *http.Response) error {
@@ -107,10 +106,14 @@ func checkValidHTTPResponse(resp *http.Response) error {
 	defer resp.Body.Close()
 	typ, _, mimeErr := mime.ParseMediaType(resp.Header.Get("Content-Type"))
 	if mimeErr != nil {
-		return newError(50000, mimeErr)
+		return &Error{
+			Code:       50000,
+			StatusCode: resp.StatusCode,
+			Err:        mimeErr,
+		}
 	}
 	if typ != protocolJSON && typ != protocolMsgPack {
-		return errFromUnprocessableBody(resp.Body)
+		return errFromUnprocessableBody(resp)
 	}
 
 	body := &errorBody{}

diff --git a/ably/error_test.go b/ably/error_test.go
@@ -1,14 +1,19 @@
 package ably_test
 
 import (
+	"crypto/tls"
+	"encoding/json"
 	"fmt"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
 	"strconv"
+	"sync/atomic"
 	"testing"
+	"time"
 
 	"github.com/ably/ably-go/ably"
+	"github.com/ably/ably-go/ably/ablytest"
 	"github.com/stretchr/testify/assert"
 )
 
@@ -54,3 +59,83 @@ func TestIssue127ErrorResponse(t *testing.T) {
 	assert.NotNil(t, err)
 	assert.Contains(t, err.Error(), errMsg)
 }
+
+func TestIssue_154(t *testing.T) {
+	var retry atomic.Value
+	incrRetry := func() {
+		if v := retry.Load(); v != nil {
+			retry.Store(v.(int) + 1)
+		} else {
+			retry.Store(int(1))
+		}
+	}
+	count := func() int {
+		if v := retry.Load(); v != nil {
+			return v.(int)
+		}
+		return 0
+	}
+	ref := "Mon Jan 2 15:04:05 MST 2006"
+	errMsg := "This is an html error body"
+	ts, err := time.Parse(time.UnixDate, ref)
+	if err != nil {
+		t.Fatal(err)
+	}
+	server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {
+		incrRetry()
+		rw.Header().Set("Content-Type", "text/html")
+		rw.WriteHeader(http.StatusBadGateway)
+		rw.Write([]byte(fmt.Sprintf("<html><head></head><body>%s</body></html>", errMsg)))
+	}))
+	var d time.Duration
+	d.Milliseconds()
+	defer server.Close()
+	defaultServer := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {
+		rw.Header().Set("Content-Type", "application/json")
+		rw.WriteHeader(http.StatusOK)
+		json.NewEncoder(rw).Encode([]int64{ts.UnixNano() / int64(time.Millisecond)})
+	}))
+	defer defaultServer.Close()
+	fallbackHosts := []string{"fallback0", "fallback1", "fallback2"}
+	opts := &ably.ClientOptions{
+		Environment:   ablytest.Environment,
+		NoTLS:         true,
+		FallbackHosts: fallbackHosts,
+		AuthOptions: ably.AuthOptions{
+			UseTokenAuth: true,
+		},
+	}
+	serverURL, _ := url.Parse(server.URL)
+	defaultURL, _ := url.Parse(defaultServer.URL)
+	proxy := func(r *http.Request) (*url.URL, error) {
+		retryCount := count()
+		if retryCount < 2 {
+			return serverURL, nil
+		} else {
+			r.Host = defaultURL.Hostname()
+			return defaultURL, nil
+		}
+	}
+
+	opts.HTTPClient = &http.Client{
+		Transport: &http.Transport{
+			Proxy:        proxy,
+			TLSNextProto: map[string]func(authority string, c *tls.Conn) http.RoundTripper{},
+		},
+	}
+	client, err := ably.NewRestClient(opts)
+	if err != nil {
+		t.Fatal(err)
+	}
+	tym, err := client.Time()
+	if err != nil {
+		t.Fatalf("didn't expect error got %v ", err)
+	}
+	if !tym.Equal(ts) {
+		t.Errorf("expected %v got %v", ts, tym)
+	}
+	c := count()
+	if c != 2 {
+		t.Errorf("expected 2 retries got %d", c)
+	}
+}