fix: improve codersdk error messages when not JSON (#4495)

cli/cliui/provisionerjob.go

@@ -134,7 +134,7 @@ func ProvisionerJob(ctx context.Context, writer io.Writer, opts ProvisionerJobOp
 
 	logs, closer, err := opts.Logs()
 	if err != nil {
-		return xerrors.Errorf("logs: %w", err)
+		return xerrors.Errorf("begin streaming logs: %w", err)
 	}
 	defer closer.Close()
 

codersdk/client.go

@@ -7,6 +7,7 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"mime"
 	"net/http"
 	"net/url"
 	"strings"
@@ -122,33 +123,54 @@ func readBodyAsError(res *http.Response) error {
 		helper = "Try logging in using 'coder login <url>'."
 	}
 
-	if strings.HasPrefix(contentType, "text/plain") {
 	resp, err := io.ReadAll(res.Body)
 	if err != nil {
 		return xerrors.Errorf("read body: %w", err)
 	}
+
+	mimeType, _, err := mime.ParseMediaType(contentType)
+	if err != nil {
+		mimeType = strings.TrimSpace(strings.Split(contentType, ";")[0])
+	}
+	if mimeType != "application/json" {
+		if len(resp) > 1024 {
+			resp = append(resp[:1024], []byte("...")...)
+		}
+		if len(resp) == 0 {
+			resp = []byte("no response body")
+		}
 		return &Error{
 			statusCode: res.StatusCode,
 			Response: Response{
-				Message: string(resp),
+				Message: "unexpected non-JSON response",
+				Detail:  string(resp),
 			},
 			Helper: helper,
 		}
 	}
 
-	//nolint:varnamelen
 	var m Response
-	err := json.NewDecoder(res.Body).Decode(&m)
+	err = json.NewDecoder(bytes.NewBuffer(resp)).Decode(&m)
 	if err != nil {
 		if errors.Is(err, io.EOF) {
-			// If no body is sent, we'll just provide the status code.
 			return &Error{
 				statusCode: res.StatusCode,
+				Response: Response{
+					Message: "empty response body",
+				},
 				Helper: helper,
 			}
 		}
 		return xerrors.Errorf("decode body: %w", err)
 	}
+	if m.Message == "" {
+		if len(resp) > 1024 {
+			resp = append(resp[:1024], []byte("...")...)
+		}
+		m.Message = fmt.Sprintf("unexpected status code %d, response has no message", res.StatusCode)
+		m.Detail = string(resp)
+	}
+
 	return &Error{
 		Response:   m,
 		statusCode: res.StatusCode,

codersdk/client_internal_test.go

@@ -0,0 +1,200 @@
+package codersdk
+
+import (
+	"bytes"
+	"encoding/json"
+	"fmt"
+	"io"
+	"net/http"
+	"net/http/httptest"
+	"strconv"
+	"strings"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"golang.org/x/xerrors"
+)
+
+const (
+	jsonCT = "application/json"
+)
+
+func Test_readBodyAsError(t *testing.T) {
+	t.Parallel()
+
+	exampleURL := "http://example.com"
+	simpleResponse := Response{
+		Message: "test",
+		Detail:  "hi",
+	}
+
+	longResponse := ""
+	for i := 0; i < 2000; i++ {
+		longResponse += "a"
+	}
+
+	unexpectedJSON := marshalJSON(map[string]any{
+		"hello": "world",
+		"foo":   "bar",
+	})
+
+	//nolint:bodyclose
+	tests := []struct {
+		name   string
+		req    *http.Request
+		res    *http.Response
+		assert func(t *testing.T, err error)
+	}{
+		{
+			name: "JSONWithRequest",
+			req:  httptest.NewRequest(http.MethodGet, exampleURL, nil),
+			res:  newResponse(http.StatusNotFound, jsonCT, marshalJSON(simpleResponse)),
+			assert: func(t *testing.T, err error) {
+				sdkErr := assertSDKError(t, err)
+
+				assert.Equal(t, simpleResponse, sdkErr.Response)
+				assert.ErrorContains(t, err, sdkErr.Response.Message)
+				assert.ErrorContains(t, err, sdkErr.Response.Detail)
+
+				assert.Equal(t, http.StatusNotFound, sdkErr.StatusCode())
+				assert.ErrorContains(t, err, strconv.Itoa(sdkErr.StatusCode()))
+
+				assert.Equal(t, http.MethodGet, sdkErr.method)
+				assert.ErrorContains(t, err, sdkErr.method)
+
+				assert.Equal(t, exampleURL, sdkErr.url)
+				assert.ErrorContains(t, err, sdkErr.url)
+
+				assert.Empty(t, sdkErr.Helper)
+			},
+		},
+		{
+			name: "JSONWithoutRequest",
+			req:  nil,
+			res:  newResponse(http.StatusNotFound, jsonCT, marshalJSON(simpleResponse)),
+			assert: func(t *testing.T, err error) {
+				sdkErr := assertSDKError(t, err)
+
+				assert.Equal(t, simpleResponse, sdkErr.Response)
+				assert.Equal(t, http.StatusNotFound, sdkErr.StatusCode())
+				assert.Empty(t, sdkErr.method)
+				assert.Empty(t, sdkErr.url)
+				assert.Empty(t, sdkErr.Helper)
+			},
+		},
+		{
+			name: "UnauthorizedHelper",
+			req:  nil,
+			res:  newResponse(http.StatusUnauthorized, jsonCT, marshalJSON(simpleResponse)),
+			assert: func(t *testing.T, err error) {
+				sdkErr := assertSDKError(t, err)
+
+				assert.Contains(t, sdkErr.Helper, "Try logging in")
+				assert.ErrorContains(t, err, sdkErr.Helper)
+			},
+		},
+		{
+			name: "NonJSON",
+			req:  nil,
+			res:  newResponse(http.StatusNotFound, "text/plain; charset=utf-8", "hello world"),
+			assert: func(t *testing.T, err error) {
+				sdkErr := assertSDKError(t, err)
+
+				assert.Contains(t, sdkErr.Response.Message, "unexpected non-JSON response")
+				assert.Equal(t, "hello world", sdkErr.Response.Detail)
+			},
+		},
+		{
+			name: "NonJSONLong",
+			req:  nil,
+			res:  newResponse(http.StatusNotFound, "text/plain; charset=utf-8", longResponse),
+			assert: func(t *testing.T, err error) {
+				sdkErr := assertSDKError(t, err)
+
+				assert.Contains(t, sdkErr.Response.Message, "unexpected non-JSON response")
+
+				expected := longResponse[0:1024] + "..."
+				assert.Equal(t, expected, sdkErr.Response.Detail)
+			},
+		},
+		{
+			name: "JSONNoBody",
+			req:  nil,
+			res:  newResponse(http.StatusNotFound, jsonCT, ""),
+			assert: func(t *testing.T, err error) {
+				sdkErr := assertSDKError(t, err)
+
+				assert.Contains(t, sdkErr.Response.Message, "empty response body")
+			},
+		},
+		{
+			name: "JSONNoMessage",
+			req:  nil,
+			res:  newResponse(http.StatusNotFound, jsonCT, unexpectedJSON),
+			assert: func(t *testing.T, err error) {
+				sdkErr := assertSDKError(t, err)
+
+				assert.Contains(t, sdkErr.Response.Message, "unexpected status code")
+				assert.Contains(t, sdkErr.Response.Message, "has no message")
+				assert.Equal(t, unexpectedJSON, sdkErr.Response.Detail)
+			},
+		},
+	}
+
+	for _, c := range tests {
+		c := c
+		t.Run(c.name, func(t *testing.T) {
+			t.Parallel()
+
+			c.res.Request = c.req
+
+			err := readBodyAsError(c.res)
+			c.assert(t, err)
+		})
+	}
+}
+
+func assertSDKError(t *testing.T, err error) *Error {
+	t.Helper()
+
+	var sdkErr *Error
+	require.Error(t, err)
+	require.True(t, xerrors.As(err, &sdkErr))
+
+	return sdkErr
+}
+
+func newResponse(status int, contentType string, body interface{}) *http.Response {
+	var r io.ReadCloser
+	switch v := body.(type) {
+	case string:
+		r = io.NopCloser(strings.NewReader(v))
+	case []byte:
+		r = io.NopCloser(bytes.NewReader(v))
+	case io.ReadCloser:
+		r = v
+	case io.Reader:
+		r = io.NopCloser(v)
+	default:
+		panic(fmt.Sprintf("unknown body type: %T", body))
+	}
+
+	return &http.Response{
+		Status:     http.StatusText(status),
+		StatusCode: status,
+		Header: http.Header{
+			"Content-Type": []string{contentType},
+		},
+		Body: r,
+	}
+}
+
+func marshalJSON(res any) string {
+	b, err := json.Marshal(res)
+	if err != nil {
+		panic(err)
+	}
+
+	return string(b)
+}