deal with 202 Accepted status code by caarlos0 · Pull Request #488 · google/go-github

caarlos0 · 2016-12-08T19:16:40Z

When github returns a 202, it means that they don't have the data you asked, and it is considered a rater heavy operation to get it, so they schedule a background job to fetch and cache it, and subsequent calls to it might work.

go-github was failing to deal with that, because in some places it was trying to parse the body to a list of some object, and github actually returns a {}.

With this change we would be able to do the same as we do with rate limiting issues, e.g.:

stats, _, err := client.Repositories.ListContributorsStats(org, repo)
if err != nil {
	if _, ok := err.(*github.AcceptedError); ok {
		// handle accepted error, likely by trying again later
	}
	// handle other errors
}

fixes #486

dmitshur

This change affects CheckResponse, which is an exported method. Its current documentation says:

// CheckResponse checks the API response for errors, and returns them if
// present.  A response is considered an error if it has a status code outside
// the 200 range.  API error responses are expected to have either no response
// body, or a JSON response body that maps to ErrorResponse.  Any other
// response body will be silently ignored.
//
// The error type will be *RateLimitError for rate limit exceeded errors,
// and *TwoFactorAuthError for two-factor authentication errors.
func CheckResponse(r *http.Response) error {

That is obviously no longer true, and needs to be updated to match reality. Specifically, this sentence "A response is considered an error if it has a status code outside the 200 range.", and the second paragraph should document that AcceptedError can be returned.

Also, you overlooked the first 3 lines of CheckResponse, which will cause this change to not work.

 if c := r.StatusCode; 200 <= c && c <= 299 {
 	return nil
 }

That'll need to be updated too.

dmitshur · 2016-12-08T19:27:51Z

github/github.go

 		}
 		return abuseRateLimitError
+	case r.StatusCode == http.StatusAccepted:
+		return &AcceptedError{}


I'm tempted to suggest we turn this into a value rather than a pointer. It's an empty struct. Pointer seems unneeded and unhelpful.

return AcceptedError{}

dmitshur · 2016-12-08T19:28:20Z

github/github.go

+type AcceptedError struct{}
+
+func (r *AcceptedError) Error() string {
+	return "Job scheduled on GitHub side, try again later"


Error strings should not be capitalized. See https://github.com/golang/go/wiki/CodeReviewComments#error-strings.

dmitshur · 2016-12-08T19:29:13Z

github/github.go

+// the information needed and cache it.
+type AcceptedError struct{}
+
+func (r *AcceptedError) Error() string {


Receiver is unused and can be removed. See rationale here.

dmitshur · 2016-12-08T19:31:57Z

github/github.go

+// AcceptedError occurs when GitHub returns 202 Accepted response with an
+// empty body, which means a job was scheduled on the GitHub side to process
+// the information needed and cache it.
+type AcceptedError struct{}


I'm not sure if it's obvious or worth adding another sentence to the current documentation, something like:

Technically, 202 Accepted is not a real error, it's just used to indicate that results are not ready yet, but should be available soon. The request should be repeated after some time.

The part that's missing to me is that the documentation should indicate that the this is a "retryable" error. Trying again after some time is completely normal and expected to succeed (but maybe not on the first try).

caarlos0 · 2016-12-09T11:45:10Z

@shurcooL thanks for the review! I think I fixed everything, I still not sure about the messages docs wording though.

Thanks!

dmitshur

This is starting to look good.

See an inline comment. Additionally, I think this PR would benefit significantly if we can document the endpoints where AcceptedError can be returned. As far as I can tell, it'd be the 5 endpoints listed at https://developer.github.com/v3/repos/statistics/. Do you know/think any others can return 202?

An alternative is to document AcceptedError in doc.go package documentation. That'd be easier, but not as helpful IMO.

dmitshur · 2016-12-09T15:11:34Z

github/github.go

 // response body will be silently ignored.
 //
 // The error type will be *RateLimitError for rate limit exceeded errors,
 // and *TwoFactorAuthError for two-factor authentication errors.


Let's mention AcceptedError here.

// The error type will be *RateLimitError for rate limit exceeded errors, // AcceptedError for 202 Accepted status codes, // and *TwoFactorAuthError for two-factor authentication errors.

This makes me realize that there's actually a good reason to revert back to using a pointer to *AcceptedError rather than value: for consistency of type assertion code to other error types. Currently, it'd be:

switch err.(type) { case *github.RateLimitError: // handle rate limit exceeded error case *github.TwoFactorAuthError: // handle two-factor authentication error case github.AcceptedError: // inconsistent // handle accepted error }

If we revert to using a pointer to *AcceptedError, it becomes consistent.

switch err.(type) { case *github.RateLimitError: // handle rate limit exceeded error case *github.TwoFactorAuthError: // handle two-factor authentication error case *github.AcceptedError: // consistent // handle accepted error }

I think that's a good argument to use pointer here too. Do you agree?

Yes, I agree... looks more consistent indeed

dmitshur · 2016-12-09T15:23:29Z

However, at this point I'd like to ping @willnorris or @gmlewis to take a look at this API change to CheckResponse and see if they agree this is an improvement. It's a slightly breaking API change (well, it changes the behavior more than API), so I wanna get their thoughts.

Also, it's kinda going against the HTTP spec which says that 2xx range status codes are "success" cases. But I think in Go it's expected/most idiomatic to return a non-nil error whenever the result you asked for cannot be returned to you.

My main argument for this change in #486 (comment) was:

Returning a non-nil error when the result that was asked for is not returned seems very reasonable to me. Otherwise every user of this library and that function will need to check both err and resp.StatusCode, which isn't as friendly.

caarlos0 · 2016-12-09T18:22:43Z

This is starting to look good.

See an inline comment. Additionally, I think this PR would benefit significantly if we can document the endpoints where AcceptedError can be returned. As far as I can tell, it'd be the 5 endpoints listed at https://developer.github.com/v3/repos/statistics/. Do you know/think any others can return 202?

An alternative is to document AcceptedError in doc.go package documentation. That'd be easier, but not as helpful IMO.

I did some search and found:

Seems like the enterprise endpoints listed here aren't implemented on go-github.

BTW I found this by searching for 202 accepted site:developer.github.com on Google.

dmitshur · 2016-12-09T18:45:27Z

I did some search and found

Thanks for doing that.

Thinking more about it, I think we should document the 202 Accepted status being translated into AcceptedError at the package level rather than individual method. It seems that this can apply to many endpoints, and it's a general behavior, not specific to a few endpoints. That way, as new endpoints suddenly decide to use 202 status, our documentation won't get outdated.

How about adding a new section to the package doc.go (also to README.md, the two should be in sync now) then. It could mention when *github.AcceptedError is returned, and a snippet how to detect it (similar to *github.RateLimitError).

Feel free to delay working on this until we hear from another maintainer of this library that this is a good change to go with.

dmitshur · 2016-12-09T18:47:12Z

github/repos_stats.go

+// repository, this method will return an *AcceptedError and a status code of
 // 202. This is because this is the status that github returns to signify that
 // it is now computing the requested statistics. A follow up request, after a
 // delay of a second or so, should result in a successful request.


Oh, I didn't see we already had that written up. In that case, it makes a lot of sense to update it as you've done, and copy it over to the other methods (as you've also already done).

Yeah, I haven't seen that before either =)

caarlos0 · 2016-12-09T19:03:56Z

@shurcooL agreed, done.

googlebot · 2016-12-10T05:26:41Z

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

Plus a few other minor changes for improved consistency.

dmitshur · 2016-12-10T05:36:29Z

README.md

-**Build Status:** [![Build Status](https://travis-ci.org/google/go-github.svg?branch=master)](https://travis-ci.org/google/go-github)  
+**Documentation:** [![GoDoc](https://godoc.org/github.com/google/go-github/github?status.svg)](https://godoc.org/github.com/google/go-github/github)
+**Mailing List:** [go-github@googlegroups.com](https://groups.google.com/group/go-github)
+**Build Status:** [![Build Status](https://travis-ci.org/google/go-github.svg?branch=master)](https://travis-ci.org/google/go-github)


You should undo these changes. I'm guessing your editor did them for you.

The two spaces at the end of those lines are significant, they mark a line break after each line.

Compare what the README looks like now:

And after this change, it's less readable:

good catch, fixed

dmitshur · 2016-12-10T05:39:26Z

@caarlos0, I've applied a few typo fixes in 50886af.

caarlos0 · 2016-12-10T14:57:45Z

@caarlos0, I've applied a few typo fixes in 50886af.

thanks for that =)

I also fixed the header of the readme.

What's been approved are the changes to my previous "request changes" review, not the entire PR.

dmitshur · 2016-12-11T00:14:29Z

To make the latest status of this PR more visible, I'm waiting to hear from @gmlewis or another maintainer on their thoughts. All my feedback so far has already been addressed.

That said, I'm more confident this is a good direction after realizing that the following is already the current behavior for ListCommitActivity and similar, as documented:

// If this is the first time these statistics are requested for the given
// repository, this method will return a non-nil error and a status code of
// 202.

So this PR is about turning "non-nil error" into "*AcceptedError", not something more drastic as I originally thought.

gmlewis · 2016-12-12T19:39:22Z

Yes, I like this and think it will be very helpful for developers.
LGTM.
Thank you, @caarlos0 and @shurcooL!
I'm going to tweak a couple minor nits like s/github/GitHub/g and then merge it.

gmlewis · 2016-12-12T19:51:45Z

Included in 231da32.

caarlos0 · 2016-12-12T20:14:54Z

Thanks @gmlewis 👍

Fixes google#486. Closes google#488. Change-Id: I31ff003552dbffaca78c740b89d54150b81c525c

caarlos0 mentioned this pull request Dec 8, 2016

Make empty responses due to 202 Accepted status code more visible. #486

Closed

deal with 202

0c2af3c

dmitshur requested changes Dec 8, 2016

View reviewed changes

improved messages, unused pointer, etc

615c613

dmitshur reviewed Dec 9, 2016

View reviewed changes

caarlos0 added 2 commits December 9, 2016 16:12

Merge branch 'master' into 202

fbb47d2

pointer again, docs

936aa01

caarlos0 added 3 commits December 9, 2016 16:29

repos stats docs

0c97936

repo forks docs

5887906

same formating

3468168

dmitshur reviewed Dec 9, 2016

View reviewed changes

docs

ba0dc2c

Minor typo fixes in README.md and doc.go.

50886af

Plus a few other minor changes for improved consistency.

dmitshur reviewed Dec 10, 2016

View reviewed changes

caarlos0 added 2 commits December 10, 2016 12:55

adding spaces back

a65a64f

typo

63045a5

dmitshur previously approved these changes Dec 11, 2016

View reviewed changes

gmlewis closed this in 231da32 Dec 12, 2016

caarlos0 deleted the 202 branch December 12, 2016 20:14

bubg-dev pushed a commit to bubg-dev/go-github that referenced this pull request Jun 16, 2017

Deal with 202 Accepted Status Code

d640b25

Fixes google#486. Closes google#488. Change-Id: I31ff003552dbffaca78c740b89d54150b81c525c

dmitshur mentioned this pull request Mar 26, 2018

In CreateFork, return the repo info when 202 Accepted occurs. #878

Merged

Conversation

caarlos0 commented Dec 8, 2016

Uh oh!

dmitshur left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

caarlos0 commented Dec 9, 2016

Uh oh!

dmitshur left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitshur commented Dec 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

caarlos0 commented Dec 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmitshur commented Dec 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

caarlos0 commented Dec 9, 2016

Uh oh!

googlebot commented Dec 10, 2016

Uh oh!

dmitshur Dec 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitshur commented Dec 10, 2016

Uh oh!

caarlos0 commented Dec 10, 2016

Uh oh!

dmitshur commented Dec 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gmlewis commented Dec 12, 2016

Uh oh!

gmlewis commented Dec 12, 2016

Uh oh!

caarlos0 commented Dec 12, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dmitshur left a comment •

edited

Loading

dmitshur commented Dec 9, 2016 •

edited

Loading

caarlos0 commented Dec 9, 2016 •

edited

Loading

dmitshur commented Dec 9, 2016 •

edited

Loading

dmitshur Dec 10, 2016 •

edited

Loading

dmitshur commented Dec 11, 2016 •

edited

Loading