Skip to content

Commit d0fb054

Browse files
deansheatherdeansheather
authored
fix: improve codersdk error messages when not JSON (#4495)
1 parent 7bc5b89 commit d0fb054

File tree

4 files changed

+233
-11
lines changed

4 files changed

+233
-11
lines changed

‎cli/cliui/provisionerjob.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -134,7 +134,7 @@ func ProvisionerJob(ctx context.Context, writer io.Writer, opts ProvisionerJobOp
134134

135135
logs, closer, err := opts.Logs()
136136
if err != nil {
137-
return xerrors.Errorf("logs: %w", err)
137+
return xerrors.Errorf("begin streaming logs: %w", err)
138138
}
139139
defer closer.Close()
140140

‎coderd/templateversions.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -421,7 +421,7 @@ func (api *API) fetchTemplateVersionDryRunJob(rw http.ResponseWriter, r *http.Re
421421
return database.ProvisionerJob{}, false
422422
}
423423

424-
// Do a workspace resource check since it's basically a workspace dry-run .
424+
// Do a workspace resource check since it's basically a workspace dry-run.
425425
if !api.Authorize(r, rbac.ActionRead,
426426
rbac.ResourceWorkspace.InOrg(templateVersion.OrganizationID).WithOwner(job.InitiatorID.String())) {
427427
httpapi.Forbidden(rw)

‎codersdk/client.go

+31-9
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
"errors"
88
"fmt"
99
"io"
10+
"mime"
1011
"net/http"
1112
"net/url"
1213
"strings"
@@ -122,33 +123,54 @@ func readBodyAsError(res *http.Response) error {
122123
helper = "Try logging in using 'coder login <url>'."
123124
}
124125

125-
if strings.HasPrefix(contentType, "text/plain") {
126-
resp, err := io.ReadAll(res.Body)
127-
if err != nil {
128-
return xerrors.Errorf("read body: %w", err)
126+
resp, err := io.ReadAll(res.Body)
127+
if err != nil {
128+
return xerrors.Errorf("read body: %w", err)
129+
}
130+
131+
mimeType, _, err := mime.ParseMediaType(contentType)
132+
if err != nil {
133+
mimeType = strings.TrimSpace(strings.Split(contentType, ";")[0])
134+
}
135+
if mimeType != "application/json" {
136+
if len(resp) > 1024 {
137+
resp = append(resp[:1024], []byte("...")...)
138+
}
139+
if len(resp) == 0 {
140+
resp = []byte("no response body")
129141
}
130142
return &Error{
131143
statusCode: res.StatusCode,
132144
Response: Response{
133-
Message: string(resp),
145+
Message: "unexpected non-JSON response",
146+
Detail: string(resp),
134147
},
135148
Helper: helper,
136149
}
137150
}
138151

139-
//nolint:varnamelen
140152
var m Response
141-
err := json.NewDecoder(res.Body).Decode(&m)
153+
err = json.NewDecoder(bytes.NewBuffer(resp)).Decode(&m)
142154
if err != nil {
143155
if errors.Is(err, io.EOF) {
144-
// If no body is sent, we'll just provide the status code.
145156
return &Error{
146157
statusCode: res.StatusCode,
147-
Helper: helper,
158+
Response: Response{
159+
Message: "empty response body",
160+
},
161+
Helper: helper,
148162
}
149163
}
150164
return xerrors.Errorf("decode body: %w", err)
151165
}
166+
if m.Message == "" {
167+
if len(resp) > 1024 {
168+
resp = append(resp[:1024], []byte("...")...)
169+
}
170+
m.Message = fmt.Sprintf("unexpected status code %d, response has no message", res.StatusCode)
171+
m.Detail = string(resp)
172+
}
173+
152174
return &Error{
153175
Response: m,
154176
statusCode: res.StatusCode,

‎codersdk/client_internal_test.go

+200
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,200 @@
1+
package codersdk
2+
3+
import (
4+
"bytes"
5+
"encoding/json"
6+
"fmt"
7+
"io"
8+
"net/http"
9+
"net/http/httptest"
10+
"strconv"
11+
"strings"
12+
"testing"
13+
14+
"github.com/stretchr/testify/assert"
15+
"github.com/stretchr/testify/require"
16+
"golang.org/x/xerrors"
17+
)
18+
19+
const (
20+
jsonCT = "application/json"
21+
)
22+
23+
func Test_readBodyAsError(t *testing.T) {
24+
t.Parallel()
25+
26+
exampleURL := "http://example.com"
27+
simpleResponse := Response{
28+
Message: "test",
29+
Detail: "hi",
30+
}
31+
32+
longResponse := ""
33+
for i := 0; i < 2000; i++ {
34+
longResponse += "a"
35+
}
36+
37+
unexpectedJSON := marshalJSON(map[string]any{
38+
"hello": "world",
39+
"foo": "bar",
40+
})
41+
42+
//nolint:bodyclose
43+
tests := []struct {
44+
name string
45+
req *http.Request
46+
res *http.Response
47+
assert func(t *testing.T, err error)
48+
}{
49+
{
50+
name: "JSONWithRequest",
51+
req: httptest.NewRequest(http.MethodGet, exampleURL, nil),
52+
res: newResponse(http.StatusNotFound, jsonCT, marshalJSON(simpleResponse)),
53+
assert: func(t *testing.T, err error) {
54+
sdkErr := assertSDKError(t, err)
55+
56+
assert.Equal(t, simpleResponse, sdkErr.Response)
57+
assert.ErrorContains(t, err, sdkErr.Response.Message)
58+
assert.ErrorContains(t, err, sdkErr.Response.Detail)
59+
60+
assert.Equal(t, http.StatusNotFound, sdkErr.StatusCode())
61+
assert.ErrorContains(t, err, strconv.Itoa(sdkErr.StatusCode()))
62+
63+
assert.Equal(t, http.MethodGet, sdkErr.method)
64+
assert.ErrorContains(t, err, sdkErr.method)
65+
66+
assert.Equal(t, exampleURL, sdkErr.url)
67+
assert.ErrorContains(t, err, sdkErr.url)
68+
69+
assert.Empty(t, sdkErr.Helper)
70+
},
71+
},
72+
{
73+
name: "JSONWithoutRequest",
74+
req: nil,
75+
res: newResponse(http.StatusNotFound, jsonCT, marshalJSON(simpleResponse)),
76+
assert: func(t *testing.T, err error) {
77+
sdkErr := assertSDKError(t, err)
78+
79+
assert.Equal(t, simpleResponse, sdkErr.Response)
80+
assert.Equal(t, http.StatusNotFound, sdkErr.StatusCode())
81+
assert.Empty(t, sdkErr.method)
82+
assert.Empty(t, sdkErr.url)
83+
assert.Empty(t, sdkErr.Helper)
84+
},
85+
},
86+
{
87+
name: "UnauthorizedHelper",
88+
req: nil,
89+
res: newResponse(http.StatusUnauthorized, jsonCT, marshalJSON(simpleResponse)),
90+
assert: func(t *testing.T, err error) {
91+
sdkErr := assertSDKError(t, err)
92+
93+
assert.Contains(t, sdkErr.Helper, "Try logging in")
94+
assert.ErrorContains(t, err, sdkErr.Helper)
95+
},
96+
},
97+
{
98+
name: "NonJSON",
99+
req: nil,
100+
res: newResponse(http.StatusNotFound, "text/plain; charset=utf-8", "hello world"),
101+
assert: func(t *testing.T, err error) {
102+
sdkErr := assertSDKError(t, err)
103+
104+
assert.Contains(t, sdkErr.Response.Message, "unexpected non-JSON response")
105+
assert.Equal(t, "hello world", sdkErr.Response.Detail)
106+
},
107+
},
108+
{
109+
name: "NonJSONLong",
110+
req: nil,
111+
res: newResponse(http.StatusNotFound, "text/plain; charset=utf-8", longResponse),
112+
assert: func(t *testing.T, err error) {
113+
sdkErr := assertSDKError(t, err)
114+
115+
assert.Contains(t, sdkErr.Response.Message, "unexpected non-JSON response")
116+
117+
expected := longResponse[0:1024] + "..."
118+
assert.Equal(t, expected, sdkErr.Response.Detail)
119+
},
120+
},
121+
{
122+
name: "JSONNoBody",
123+
req: nil,
124+
res: newResponse(http.StatusNotFound, jsonCT, ""),
125+
assert: func(t *testing.T, err error) {
126+
sdkErr := assertSDKError(t, err)
127+
128+
assert.Contains(t, sdkErr.Response.Message, "empty response body")
129+
},
130+
},
131+
{
132+
name: "JSONNoMessage",
133+
req: nil,
134+
res: newResponse(http.StatusNotFound, jsonCT, unexpectedJSON),
135+
assert: func(t *testing.T, err error) {
136+
sdkErr := assertSDKError(t, err)
137+
138+
assert.Contains(t, sdkErr.Response.Message, "unexpected status code")
139+
assert.Contains(t, sdkErr.Response.Message, "has no message")
140+
assert.Equal(t, unexpectedJSON, sdkErr.Response.Detail)
141+
},
142+
},
143+
}
144+
145+
for _, c := range tests {
146+
c := c
147+
t.Run(c.name, func(t *testing.T) {
148+
t.Parallel()
149+
150+
c.res.Request = c.req
151+
152+
err := readBodyAsError(c.res)
153+
c.assert(t, err)
154+
})
155+
}
156+
}
157+
158+
func assertSDKError(t *testing.T, err error) *Error {
159+
t.Helper()
160+
161+
var sdkErr *Error
162+
require.Error(t, err)
163+
require.True(t, xerrors.As(err, &sdkErr))
164+
165+
return sdkErr
166+
}
167+
168+
func newResponse(status int, contentType string, body interface{}) *http.Response {
169+
var r io.ReadCloser
170+
switch v := body.(type) {
171+
case string:
172+
r = io.NopCloser(strings.NewReader(v))
173+
case []byte:
174+
r = io.NopCloser(bytes.NewReader(v))
175+
case io.ReadCloser:
176+
r = v
177+
case io.Reader:
178+
r = io.NopCloser(v)
179+
default:
180+
panic(fmt.Sprintf("unknown body type: %T", body))
181+
}
182+
183+
return &http.Response{
184+
Status: http.StatusText(status),
185+
StatusCode: status,
186+
Header: http.Header{
187+
"Content-Type": []string{contentType},
188+
},
189+
Body: r,
190+
}
191+
}
192+
193+
func marshalJSON(res any) string {
194+
b, err := json.Marshal(res)
195+
if err != nil {
196+
panic(err)
197+
}
198+
199+
return string(b)
200+
}

0 commit comments

Comments
 (0)