Skip to content
This repository has been archived by the owner on Apr 23, 2021. It is now read-only.

Expose the status of requests #37

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Expose the status of requests #37

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 8 additions & 1 deletion gcm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,12 +107,13 @@ type XmppMessage struct {

// HttpResponse is the GCM connection server response to an HTTP downstream message request.
type HttpResponse struct {
Status int `json:"-"`
MulticastId int `json:"multicast_id,omitempty"`
Success uint `json:"success,omitempty"`
Failure uint `json:"failure,omitempty"`
CanonicalIds uint `json:"canonical_ids,omitempty"`
Results []Result `json:"results,omitempty"`
MessageId int `json:"message_id,omitempty"`
MessageId int `json:"message_id,omitempty"`
Error string `json:"error,omitempty"`
}

Expand Down Expand Up @@ -194,6 +195,12 @@ func (c *httpGcmClient) send(apiKey string, m HttpMessage) (*HttpResponse, error
return nil, fmt.Errorf("error sending request to HTTP connection server>%v", err)
}
gcmResp := &HttpResponse{}
gcmResp.Status = httpResp.StatusCode

if gcmResp.Status != http.StatusOK {
return gcmResp, fmt.Errorf("Encountered HTTP status code %d", gcmResp.Status)
Copy link
Contributor

Choose a reason for hiding this comment

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

@jpatel531 You are ditching the response body, which can contain useful info. A better way on my opinion is to always read the body, try to unmarshal it, and if failed return the body as is.

Copy link
Author

Choose a reason for hiding this comment

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

Fair point @bolshoy - although from reading the reference ( https://developers.google.com/cloud-messaging/http-server-ref#error-codes ) it doesn't look like non-200 bodies contain anything particularly interesting. How would you suggest we return a raw body? Include a RawBody []byte in the HttpResponse struct?

Copy link
Contributor

@bolshoy bolshoy Jun 30, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

@jpatel531 I don't think the upstream client is really interested in the response status code and its meaning, it needs either a response object or an error explaining why there's no object. So in case of http statuses 400,401,5xx in the spec, it would be something along the lines of

gcmResp := &HttpResponse{}
body, err := ioutil.ReadAll(httpResp.Body)
defer httpResp.Body.Close()
if err != nil {
        return nil, fmt.Errorf("error reading http response body: %v", err)
}
if httpResp.StatusCode >= http.StatusBadRequest { // Not sure about 3xx 
        return nil, fmt.Errorf(fmt.Sprintf("http error: %s (%s)", httpResp.Status, string(body)))
}
err = json.Unmarshal(body, &gcmResp)
if err != nil {
        return nil, fmt.Errorf("error unmarshaling json from body: %v %s", err, string(body))
}

}

body, err := ioutil.ReadAll(httpResp.Body)
defer httpResp.Body.Close()
if err != nil {
Expand Down
1 change: 1 addition & 0 deletions gcm_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,7 @@ func TestHttpClientSend(t *testing.T) {
expectedAuthHeader := "key=apiKey"
expResp := &HttpResponse{}
err := json.Unmarshal([]byte(expectedResp), &expResp)
expResp.Status = http.StatusOK
if err != nil {
t.Fatalf("error: %v", err)
}
Expand Down