Merge pull request mattbaird#256 from maraino/master

Do not attempt to parse non json errors

lib/request.go

@@ -18,6 +18,7 @@ import (
 	"fmt"
 	"io"
 	"io/ioutil"
+	"mime"
 	"net/http"
 	"net/url"
 	"reflect"
@@ -130,6 +131,17 @@ func (r *Request) DoResponse(v interface{}) (*http.Response, []byte, error) {
 	}
 
 	if res.StatusCode > 304 && v != nil {
+		// Make sure the response is JSON and not some other type.
+		// i.e. 502 or 504 errors from a proxy.
+		mediaType, _, err := mime.ParseMediaType(res.Header.Get("Content-Type"))
+		if err != nil {
+			return nil, bodyBytes, err
+		}
+
+		if mediaType != "application/json" {
+			return nil, bodyBytes, fmt.Errorf(http.StatusText(res.StatusCode))
+		}
+
 		jsonErr := json.Unmarshal(bodyBytes, v)
 		if jsonErr != nil {
 			return nil, nil, fmt.Errorf("Json response unmarshal error: [%s], response content: [%s]", jsonErr.Error(), string(bodyBytes))

lib/request_test.go

@@ -78,6 +78,71 @@ func TestQueryString(t *testing.T) {
 	assert.T(t, err != nil, fmt.Sprintf("Expected err to not be nil"))
 }
 
+func TestDoResponseError(t *testing.T) {
+	v := make(map[string]string)
+	conn := NewConn()
+	req, _ := conn.NewRequest("GET", "http://mock.com", "")
+	req.Client = http.DefaultClient
+	defer func() {
+		req.Client.Transport = http.DefaultTransport
+	}()
+
+	// application/json
+	req.Client.Transport = newMockTransport(500, "application/json", `{"error":"internal_server_error"}`)
+	res, bodyBytes, err := req.DoResponse(&v)
+	assert.NotEqual(t, nil, res)
+	assert.Equal(t, nil, err)
+	assert.Equal(t, 500, res.StatusCode)
+	assert.Equal(t, "application/json", res.Header.Get("Content-Type"))
+	assert.Equal(t, "internal_server_error", v["error"])
+	assert.Equal(t, []byte(`{"error":"internal_server_error"}`), bodyBytes)
+
+	// text/html
+	v = make(map[string]string)
+	req.Client.Transport = newMockTransport(500, "text/html", "HTTP 500 Internal Server Error")
+	res, bodyBytes, err = req.DoResponse(&v)
+	assert.T(t, res == nil, fmt.Sprintf("Expected nil, got: %v", res))
+	assert.NotEqual(t, nil, err)
+	assert.Equal(t, 0, len(v))
+	assert.Equal(t, []byte("HTTP 500 Internal Server Error"), bodyBytes)
+	assert.Equal(t, fmt.Errorf(http.StatusText(500)), err)
+
+	//  mime error
+	v = make(map[string]string)
+	req.Client.Transport = newMockTransport(500, "", "HTTP 500 Internal Server Error")
+	res, bodyBytes, err = req.DoResponse(&v)
+	assert.T(t, res == nil, fmt.Sprintf("Expected nil, got: %v", res))
+	assert.NotEqual(t, nil, err)
+	assert.Equal(t, 0, len(v))
+	assert.Equal(t, []byte("HTTP 500 Internal Server Error"), bodyBytes)
+	assert.NotEqual(t, fmt.Errorf(http.StatusText(500)), err)
+}
+
+type mockTransport struct {
+	statusCode  int
+	contentType string
+	body        string
+}
+
+func newMockTransport(statusCode int, contentType, body string) http.RoundTripper {
+	return &mockTransport{
+		statusCode:  statusCode,
+		contentType: contentType,
+		body:        body,
+	}
+}
+
+func (t *mockTransport) RoundTrip(req *http.Request) (*http.Response, error) {
+	response := &http.Response{
+		Header:     make(http.Header),
+		Request:    req,
+		StatusCode: t.statusCode,
+	}
+	response.Header.Set("Content-Type", t.contentType)
+	response.Body = ioutil.NopCloser(strings.NewReader(t.body))
+	return response, nil
+}
+
 func TestSetBodyGzip(t *testing.T) {
 	s := "foo"