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

Expose the status of requests - #37

Open
jpatel531 wants to merge 1 commit into
googlearchive:masterfrom
jpatel531:expose-status
Open

Expose the status of requests#37
jpatel531 wants to merge 1 commit into
googlearchive:masterfrom
jpatel531:expose-status

Conversation

@jpatel531

@jpatel531 jpatel531 commented Jun 29, 2016

Copy link
Copy Markdown

Hi there,

Whenever I got 401 errors, the library would panic. The GCM response for 401s are in HTML, whereas the library was trying to parse the body from JSON regardless of the status.

This PR adds a Status field to the HttpResponse struct, and also returns an error upon a non-200 response.

Let me know what you think

@googlebot

Copy link
Copy Markdown

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@jpatel531

Copy link
Copy Markdown
Author

I've also signed the license agreement

@googlebot

Copy link
Copy Markdown

CLAs look good, thanks!

Comment thread gcm.go
gcmResp.Status = httpResp.StatusCode

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

Copy link
Copy Markdown
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
Copy Markdown
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?

@bolshoy bolshoy Jun 30, 2016

Copy link
Copy Markdown
Contributor

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))
}

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants