-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce api.Error type (and improve errors) #64
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,7 +8,6 @@ package api | |
import ( | ||
"bytes" | ||
"encoding/json" | ||
"errors" | ||
"fmt" | ||
"io/ioutil" | ||
"net/http" | ||
|
@@ -52,7 +51,7 @@ func (m HTTPMethod) String() (string, error) { | |
case PutReq: | ||
return "PUT", nil | ||
default: | ||
return "", errors.New("Invalid HTTP method specified") | ||
return "", fmt.Errorf("invalid HTTP method specified: %q", m) | ||
} | ||
} | ||
|
||
|
@@ -135,6 +134,7 @@ type Requester interface { | |
|
||
// authedRequester answers API requests by making an authed API call. | ||
type authedRequester struct { | ||
client http.Client | ||
apikey string | ||
baseurl url.URL | ||
} | ||
|
@@ -178,38 +178,38 @@ func (s *authedRequester) Do(r *Request) *Response { | |
theurl.RawQuery = encodedParams | ||
} | ||
req, err := http.NewRequest(reqMethod, theurl.String(), bytes.NewReader(r.Body.Bytes())) | ||
if err != nil { | ||
return &Response{err: fmt.Errorf("failed to build HTTP request: %w", err)} | ||
} | ||
|
||
// Specify content type for POST requests, as the body format has to be specified | ||
if r.ReqType == PostReq { | ||
req.Header.Set("Content-Type", "application/x-www-form-urlencoded; param=value") | ||
} | ||
|
||
res, err := s.client.Do(req) | ||
if err != nil { | ||
return &Response{err: err} | ||
} | ||
client := &http.Client{} | ||
res, err := client.Do(req) | ||
if err != nil { | ||
return &Response{err: err} | ||
return &Response{err: fmt.Errorf("failed to perform HTTP request: %w", err)} | ||
} | ||
defer res.Body.Close() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are there any errors that need to be caught on closing a request body? |
||
data, err := ioutil.ReadAll(res.Body) | ||
if err != nil { | ||
return &Response{err: err} | ||
} | ||
if res.StatusCode != 200 { | ||
return &Response{err: fmt.Errorf("%s Not ok: HTTP %d\n%s", r.Endpoint, res.StatusCode, string(data))} | ||
return &Response{err: fmt.Errorf("failed to read HTTP response: %w", err)} | ||
} | ||
|
||
// Even if the result is non-200 we still want to try to unmarshal the response, to have a more descriptive error | ||
var response struct { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are there any situations where the response coming back from MyRadio states that it is not OK in the payload but OK in the HTTP status, or vice versa? Am just wondering about the fact that there is now only one check at the response body level and nothing on the HTTP code directly. (Presumably those would be bugs in MyRadio, mind. It seems kind of weird to have that redundancy, but it's been so long since I looked at MyRadio that I don't even know anymore.) |
||
Status string | ||
Payload *json.RawMessage | ||
} | ||
err = json.Unmarshal(data, &response) | ||
if err != nil { | ||
return &Response{err: err} | ||
return &Response{err: fmt.Errorf("failed to parse HTTP response: %w", err)} | ||
} | ||
if response.Status != "OK" { | ||
return &Response{err: fmt.Errorf(r.Endpoint + fmt.Sprintf(" Response not OK: %v", response))} | ||
var payloadStr string | ||
_ = json.Unmarshal(*response.Payload, &payloadStr) | ||
return &Response{err: Error{Endpoint: r.Endpoint, Code: res.StatusCode, Payload: payloadStr}} | ||
} | ||
return &Response{raw: response.Payload, err: nil} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
package api | ||
|
||
import "fmt" | ||
|
||
// Error is the error type returned when API requests result in an error. | ||
type Error struct { | ||
Endpoint string | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could the fields be commented, eg. making clear that |
||
Code int | ||
Payload string | ||
} | ||
|
||
func (a Error) Error() string { | ||
return fmt.Sprintf("%s NOT OK: %d: %s", a.Endpoint, a.Code, a.Payload) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I presume the old behaviour of setting the
Content-Type
header before bailing for the error was a bug?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I presume it would have been, I would imagine that
NewRequest
would return(nil, error)
and thus we'd try to accessHeader
on anil
req
.