Skip to content
This repository was archived by the owner on Aug 30, 2024. It is now read-only.

Adjust HTTPError to read response body before it is closed #361

Merged
merged 2 commits into from
May 27, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion coder-sdk/activity.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ func (c *DefaultClient) PushActivity(ctx context.Context, source, workspaceID st
}

if resp.StatusCode != http.StatusOK {
return bodyError(resp)
return NewHTTPError(resp)
}
return nil
}
40 changes: 31 additions & 9 deletions coder-sdk/error.go
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
package coder

import (
"bytes"
"encoding/json"
"fmt"
"io"
"net/http"

"golang.org/x/xerrors"
Expand All @@ -29,11 +31,31 @@ type APIErrorMsg struct {
Details json.RawMessage `json:"details"`
}

// NewHTTPError reads the response body and stores metadata
// about the response in order to be unpacked into
// an *APIError.
func NewHTTPError(resp *http.Response) *HTTPError {
var buf bytes.Buffer
_, err := io.Copy(&buf, resp.Body)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 nice simplification. I can't think of a case where the error body would be excessively large.

if err != nil {
return &HTTPError{
cachedErr: err,
}
}
return &HTTPError{
url: resp.Request.URL.String(),
statusCode: resp.StatusCode,
body: buf.Bytes(),
}
}

// HTTPError represents an error from the Coder API.
type HTTPError struct {
*http.Response
cached *APIError
cachedErr error
url string
statusCode int
body []byte
cached *APIError
cachedErr error
}

// Payload decode the response body into the standard error structure. The `details`
Expand All @@ -46,7 +68,7 @@ func (e *HTTPError) Payload() (*APIError, error) {

// Try to decode the payload as an error, if it fails or if there is no error message,
// return the response URL with the status.
if err := json.NewDecoder(e.Response.Body).Decode(&msg); err != nil {
if err := json.Unmarshal(e.body, &msg); err != nil {
e.cachedErr = err
return nil, err
}
Expand All @@ -55,16 +77,16 @@ func (e *HTTPError) Payload() (*APIError, error) {
return &msg, nil
}

func (e *HTTPError) StatusCode() int {
return e.statusCode
}

func (e *HTTPError) Error() string {
apiErr, err := e.Payload()
if err != nil || apiErr.Err.Msg == "" {
return fmt.Sprintf("%s: %d %s", e.Request.URL, e.StatusCode, e.Status)
return fmt.Sprintf("%s: %d %s", e.url, e.statusCode, http.StatusText(e.statusCode))
}

// If the payload was a in the expected error format with a message, include it.
return apiErr.Err.Msg
}

func bodyError(resp *http.Response) error {
return &HTTPError{Response: resp}
}
2 changes: 1 addition & 1 deletion coder-sdk/request.go
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ func (c *DefaultClient) requestBody(ctx context.Context, method, path string, in
// Responses in the 100 are handled by the http lib, in the 200 range, we have a success.
// Consider anything at or above 300 to be an error.
if resp.StatusCode > 299 {
return fmt.Errorf("unexpected status code %d: %w", resp.StatusCode, bodyError(resp))
return fmt.Errorf("unexpected status code %d: %w", resp.StatusCode, NewHTTPError(resp))
}

// If we expect a payload, process it as json.
Expand Down
2 changes: 1 addition & 1 deletion coder-sdk/ws.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ func (c *DefaultClient) dialWebsocket(ctx context.Context, path string, options
conn, resp, err := websocket.Dial(ctx, url.String(), &websocket.DialOptions{HTTPHeader: headers})
if err != nil {
if resp != nil {
return nil, bodyError(resp)
return nil, NewHTTPError(resp)
}
return nil, err
}
Expand Down
2 changes: 1 addition & 1 deletion internal/cmd/auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ func newClient(ctx context.Context, checkVersion bool) (coder.Client, error) {
if err != nil {
var he *coder.HTTPError
if xerrors.As(err, &he) {
if he.StatusCode == http.StatusUnauthorized {
if he.StatusCode() == http.StatusUnauthorized {
return nil, xerrors.Errorf("not authenticated: try running \"coder login`\"")
}
}
Expand Down
2 changes: 1 addition & 1 deletion internal/cmd/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ func handleAPIError(origError error) error {
return origError
}

return clog.Error(fmt.Sprintf("Precondition Error : Status Code=%d", httpError.StatusCode),
return clog.Error(fmt.Sprintf("Precondition Error : Status Code=%d", httpError.StatusCode()),
p.Message,
clog.BlankLine,
clog.Tipf(p.Solution))
Expand Down
4 changes: 1 addition & 3 deletions wsnet/dial.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,7 @@ func DialWebsocket(ctx context.Context, broker string, iceServers []webrtc.ICESe
defer func() {
_ = resp.Body.Close()
}()
return nil, &coder.HTTPError{
Response: resp,
}
return nil, coder.NewHTTPError(resp)
}
return nil, fmt.Errorf("dial websocket: %w", err)
}
Expand Down
4 changes: 1 addition & 3 deletions wsnet/listen.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,9 +81,7 @@ func (l *listener) dial(ctx context.Context) (<-chan error, error) {
conn, resp, err := websocket.Dial(ctx, l.broker, nil)
if err != nil {
if resp != nil {
return nil, &coder.HTTPError{
Response: resp,
}
return nil, coder.NewHTTPError(resp)
}
return nil, err
}
Expand Down