Skip to content
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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
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
28 changes: 14 additions & 14 deletions api/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ package api
import (
"bytes"
"encoding/json"
"errors"
"fmt"
"io/ioutil"
"net/http"
Expand Down Expand Up @@ -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)
}
}

Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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 {
markspolakovs marked this conversation as resolved.
Show resolved Hide resolved
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()
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

Choose a reason for hiding this comment

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

https://stackoverflow.com/a/47294087/2586553

There are two things to consider: What would you do with it if you checked it and there was an error? And, what would the side-effects be if there was an error?

In most cases, for closing a response body, the answer to both questions is... absolutely nothing. If there's nothing you'd do if there was an error and the error has no appreciable impact, there's no reason to check it.

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 {
Copy link
Member

Choose a reason for hiding this comment

The 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.)

Copy link
Member Author

Choose a reason for hiding this comment

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

As far as I can tell, per controllers/api/v2.php any successful response will have "status": "OK" - though the converse isn't guaranteed (there are status values other than FAIL - see MyRadioException).

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}
}
Expand Down
14 changes: 14 additions & 0 deletions api/errors.go
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
markspolakovs marked this conversation as resolved.
Show resolved Hide resolved
Code int
Payload string
}

func (a Error) Error() string {
return fmt.Sprintf("%s NOT OK: %d: %s", a.Endpoint, a.Code, a.Payload)
}