From fcca88dae89534ee280be25b260e90f686b5215b Mon Sep 17 00:00:00 2001 From: Dean Sheather Date: Tue, 11 Oct 2022 20:09:04 +0000 Subject: [PATCH 1/4] fix: improve codersdk error messages when not JSON --- coderd/templateversions.go | 2 +- codersdk/client.go | 31 +++++++++++--- codersdk/client_test.go | 87 ++++++++++++++++++++++++++++++++++++++ scripts/develop.sh | 2 +- 4 files changed, 113 insertions(+), 9 deletions(-) create mode 100644 codersdk/client_test.go diff --git a/coderd/templateversions.go b/coderd/templateversions.go index 85494949e97dc..6207c1e3be20a 100644 --- a/coderd/templateversions.go +++ b/coderd/templateversions.go @@ -420,7 +420,7 @@ func (api *API) fetchTemplateVersionDryRunJob(rw http.ResponseWriter, r *http.Re httpapi.Forbidden(rw) return database.ProvisionerJob{}, false } - // Do a workspace resource check since it's basically a workspace dry-run . + // Do a workspace resource check since it's basically a workspace dry-run. if !api.Authorize(r, rbac.ActionRead, rbac.ResourceWorkspace.InOrg(templateVersion.OrganizationID).WithOwner(job.InitiatorID.String())) { httpapi.Forbidden(rw) diff --git a/codersdk/client.go b/codersdk/client.go index 0c3d89a92843b..39cf688711d2b 100644 --- a/codersdk/client.go +++ b/codersdk/client.go @@ -7,6 +7,7 @@ import ( "errors" "fmt" "io" + "mime" "net/http" "net/url" "strings" @@ -119,23 +120,31 @@ func readBodyAsError(res *http.Response) error { helper = "Try logging in using 'coder login '." } - if strings.HasPrefix(contentType, "text/plain") { - resp, err := io.ReadAll(res.Body) - if err != nil { - return xerrors.Errorf("read body: %w", err) + 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("...")...) } 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. @@ -146,6 +155,14 @@ func readBodyAsError(res *http.Response) error { } 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, diff --git a/codersdk/client_test.go b/codersdk/client_test.go new file mode 100644 index 0000000000000..55922a84ecec1 --- /dev/null +++ b/codersdk/client_test.go @@ -0,0 +1,87 @@ +package codersdk_test + +import ( + "bytes" + "encoding/json" + "fmt" + "io" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/coder/coder/codersdk" +) + +const ( + jsonCT = "application/json" +) + +func Test_readBodyAsError(t *testing.T) { + simpleResponse := codersdk.Response{ + Message: "test", + Detail: "hi", + } + + tests := []struct { + name string + req *http.Request + res *http.Response + errContains error + assert func(t *testing.T, err *codersdk.Error) + }{ + { + name: "StandardWithRequest", + req: httptest.NewRequest(http.MethodGet, "http://example.com", nil), + res: newResponse(http.StatusNotFound, jsonCT, responseString(simpleResponse)), + assert: func(t *testing.T, err *codersdk.Error) { + assert.Equal(t, http.StatusNotFound, err.StatusCode()) + assert.Equal(t, simpleResponse, err.Response) + // assert.Equal(t, http.MethodGet, err.) + }, + }, + } + for _, c := range tests { + t.Run(c.name, func(t *testing.T) { + t.Parallel() + + // TODO: this + }) + } +} + +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 responseString(res codersdk.Response) string { + b, err := json.Marshal(res) + if err != nil { + panic(err) + } + + return string(b) +} diff --git a/scripts/develop.sh b/scripts/develop.sh index a1ab8a884cf4d..16e0c170955b4 100755 --- a/scripts/develop.sh +++ b/scripts/develop.sh @@ -59,7 +59,7 @@ CODER_DEV_SHIM="${PROJECT_ROOT}/scripts/coder-dev.sh" # rather than leaving things in an inconsistent state. trap 'kill -TERM -$$' ERR cdroot - "${CODER_DEV_SHIM}" server --address 0.0.0.0:3000 || kill -INT -$$ & + "${CODER_DEV_SHIM}" server --address 0.0.0.0:3000 --auto-import-template kubernetes || kill -INT -$$ & echo '== Waiting for Coder to become ready' timeout 60s bash -c 'until curl -s --fail http://localhost:3000 > /dev/null 2>&1; do sleep 0.5; done' From ea234fcbe69ebd95a852212a6888d27b8eb8c0df Mon Sep 17 00:00:00 2001 From: Dean Sheather Date: Tue, 11 Oct 2022 20:11:21 +0000 Subject: [PATCH 2/4] fixup! fix: improve codersdk error messages when not JSON --- scripts/develop.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/develop.sh b/scripts/develop.sh index 16e0c170955b4..a1ab8a884cf4d 100755 --- a/scripts/develop.sh +++ b/scripts/develop.sh @@ -59,7 +59,7 @@ CODER_DEV_SHIM="${PROJECT_ROOT}/scripts/coder-dev.sh" # rather than leaving things in an inconsistent state. trap 'kill -TERM -$$' ERR cdroot - "${CODER_DEV_SHIM}" server --address 0.0.0.0:3000 --auto-import-template kubernetes || kill -INT -$$ & + "${CODER_DEV_SHIM}" server --address 0.0.0.0:3000 || kill -INT -$$ & echo '== Waiting for Coder to become ready' timeout 60s bash -c 'until curl -s --fail http://localhost:3000 > /dev/null 2>&1; do sleep 0.5; done' From c2e3df3b868ba7d7b749398c26436728a95eb837 Mon Sep 17 00:00:00 2001 From: Dean Sheather Date: Wed, 12 Oct 2022 16:03:23 +0000 Subject: [PATCH 3/4] fixup! fix: improve codersdk error messages when not JSON --- cli/cliui/provisionerjob.go | 2 +- codersdk/client.go | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/cli/cliui/provisionerjob.go b/cli/cliui/provisionerjob.go index c781be74f2cc1..1fb7025f2cbb4 100644 --- a/cli/cliui/provisionerjob.go +++ b/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() diff --git a/codersdk/client.go b/codersdk/client.go index 39cf688711d2b..fa762636fcb61 100644 --- a/codersdk/client.go +++ b/codersdk/client.go @@ -133,6 +133,9 @@ func readBodyAsError(res *http.Response) error { 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{ From e8f2b13cfb7affcfe686faf59b5f9ec0750edba3 Mon Sep 17 00:00:00 2001 From: Dean Sheather Date: Tue, 18 Oct 2022 21:23:07 +0000 Subject: [PATCH 4/4] chore: tests for codersdk readBodyAsError --- codersdk/client.go | 6 +- codersdk/client_internal_test.go | 200 +++++++++++++++++++++++++++++++ codersdk/client_test.go | 87 -------------- 3 files changed, 204 insertions(+), 89 deletions(-) create mode 100644 codersdk/client_internal_test.go delete mode 100644 codersdk/client_test.go diff --git a/codersdk/client.go b/codersdk/client.go index fa762636fcb61..a5372dee9e500 100644 --- a/codersdk/client.go +++ b/codersdk/client.go @@ -150,10 +150,12 @@ func readBodyAsError(res *http.Response) error { 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, - Helper: helper, + Response: Response{ + Message: "empty response body", + }, + Helper: helper, } } return xerrors.Errorf("decode body: %w", err) diff --git a/codersdk/client_internal_test.go b/codersdk/client_internal_test.go new file mode 100644 index 0000000000000..f4cb49297c0a0 --- /dev/null +++ b/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) +} diff --git a/codersdk/client_test.go b/codersdk/client_test.go deleted file mode 100644 index 55922a84ecec1..0000000000000 --- a/codersdk/client_test.go +++ /dev/null @@ -1,87 +0,0 @@ -package codersdk_test - -import ( - "bytes" - "encoding/json" - "fmt" - "io" - "net/http" - "net/http/httptest" - "strings" - "testing" - - "github.com/stretchr/testify/assert" - - "github.com/coder/coder/codersdk" -) - -const ( - jsonCT = "application/json" -) - -func Test_readBodyAsError(t *testing.T) { - simpleResponse := codersdk.Response{ - Message: "test", - Detail: "hi", - } - - tests := []struct { - name string - req *http.Request - res *http.Response - errContains error - assert func(t *testing.T, err *codersdk.Error) - }{ - { - name: "StandardWithRequest", - req: httptest.NewRequest(http.MethodGet, "http://example.com", nil), - res: newResponse(http.StatusNotFound, jsonCT, responseString(simpleResponse)), - assert: func(t *testing.T, err *codersdk.Error) { - assert.Equal(t, http.StatusNotFound, err.StatusCode()) - assert.Equal(t, simpleResponse, err.Response) - // assert.Equal(t, http.MethodGet, err.) - }, - }, - } - for _, c := range tests { - t.Run(c.name, func(t *testing.T) { - t.Parallel() - - // TODO: this - }) - } -} - -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 responseString(res codersdk.Response) string { - b, err := json.Marshal(res) - if err != nil { - panic(err) - } - - return string(b) -}