Repository: ably/ably-go (go)

Issue:
Lib failing to retrying on 5xx if it can't parse the body
[Customer using ably-go 1.1.2](https://support.ably.io/a/tickets/106111) reporting 400 errors. On investigation this was due to cloudfront returning rendered HTML for 502 errors; ably-go is then unable to parse the body, and turns it into a 400 error.

But this is incorrect - the library should be retrying for any response with an http status code 500-504 (specific spec violation: https://docs.ably.io/client-lib-development-guide/features/#RSC15d), doesn't matter what the body is. It still has the HTTP status code, and can decide to retry based on that.

Possibly introduced by https://github.com/ably/ably-go/pull/129/files (though that may have been fixing even-more-broken older behaviour)

Pull request description:
Properly set  Error.StatusCode
fixes #154

newError was completely ignoring  response status code.
It was wrongly deriving it from error code.

This patch ensures Error,StatusCode is set with correct response status.

Patch:
diff --git a/ably/error.go b/ably/error.go
index 132e84d..f306a9b 100644
--- 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{}


Test patch:
diff --git a/ably/error_test.go b/ably/error_test.go
index a3ee1ff..499935b 100644
--- a/ably/error_test.go
+++ b/ably/error_test.go
@@ -54,3 +54,28 @@ func TestIssue127ErrorResponse(t *testing.T) {
 	assert.NotNil(t, err)
 	assert.Contains(t, err.Error(), errMsg)
 }
+func TestIssue_154(t *testing.T) {
+	server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {
+		rw.Header().Set("Content-Type", "text/html")
+		rw.WriteHeader(http.StatusMethodNotAllowed)
+	}))
+	defer server.Close()
+
+	endpointURL, err := url.Parse(server.URL)
+	assert.Nil(t, err)
+	opts := ably.NewClientOptions("xxxxxxx.yyyyyyy:zzzzzzz")
+	opts.NoTLS = true
+	opts.UseTokenAuth = true
+	opts.RestHost = endpointURL.Hostname()
+	port, _ := strconv.ParseInt(endpointURL.Port(), 10, 0)
+	opts.Port = int(port)
+	client, e := ably.NewRestClient(opts)
+	assert.Nil(t, e)
+
+	_, err = client.Time()
+	assert.NotNil(t, err)
+	et := err.(*ably.Error)
+	if et.StatusCode != http.StatusMethodNotAllowed {
+		t.Errorf("expected %d got %d", http.StatusMethodNotAllowed, et.StatusCode)
+	}
+}