Test repository redirects

[willnorris]

http.Client should handle following the redirects for us, but we should add a test case anyway.

Announcement: https://developer.github.com/changes/2015-04-17-preview-repository-redirects/

[huytd]

I think to test this case, we need to have a real renamed or transferred repo, so we should do it in integration test. Am I right? @willnorris

[willnorris]

Yes, an integration test would certainly be good, but we could also add a unit test where the test http server returns a redirect to a known URL and we verify that the client follows it. We'd want to inspect the exact response that GitHub returns and have our test case mimic that.

[willnorris]

So something like:

mux.HandleFunc("/repos/o/r1", func(...) {
    testMethod(t, r, "GET")
    http.Redirect(w, r, "/repos/o/r2", http.StatusFound)
})

mux.HandleFunc("/repos/o/r2", func(...) {
    testMethod(t, r, "GET")
    fmt.Fprint(w, `{"id":1}`)
})

Then test that calling client.Repositories.Get("o", "r1") returns a Repository with ID == 1. Or if GitHub actually uses a different status code, then we should use that here as well

[huytd]

Oh I see. Thank you so much! In this case, unit test is better. I think I can do this issue 👍

[willnorris]

Cool. I've invited you to collaborate on the repo. Once you accept that, I'll assign this issue to you.

[huytd]

That's great! Thank you so much!

[willnorris]

So here's a real response from GitHub. We should do basically the same in our test... return a 301 status and maybe include a basic JSON response body to make sure the library handles that okay. What's also particularly interesting is that the redirect URL is of the form /repositories/{id} rather than /repos/{owner}/{repo}, so we should mimic that behavior as well.

% curl -i "https://api.github.com/repos/googlecloudplatform/kubernetes"
HTTP/1.1 301 Moved Permanently
Server: GitHub.com
Date: Tue, 19 Jul 2016 17:57:25 GMT
Content-Type: application/json; charset=utf-8
Content-Length: 168
Status: 301 Moved Permanently
X-RateLimit-Limit: 60
X-RateLimit-Remaining: 58
X-RateLimit-Reset: 1468954348
Location: https://api.github.com/repositories/20580498
X-GitHub-Media-Type: github.v3
Access-Control-Expose-Headers: ETag, Link, X-GitHub-OTP, X-RateLimit-Limit, X-RateLimit-Remaining, X-RateLimit-Reset, X-OAuth-Scopes, X-Accepted-OAuth-Scopes, X-Poll-Interval
Access-Control-Allow-Origin: *
Content-Security-Policy: default-src 'none'
Strict-Transport-Security: max-age=31536000; includeSubdomains; preload
X-Content-Type-Options: nosniff
X-Frame-Options: deny
X-XSS-Protection: 1; mode=block
Vary: Accept-Encoding
X-Served-By: 4c8b2d4732c413f4b9aefe394bd65569
X-GitHub-Request-Id: 68840151:22AD:478E44A:578E6A05

{
  "message": "Moved Permanently",
  "url": "https://api.github.com/repositories/20580498",
  "documentation_url": "https://developer.github.com/v3/#http-redirects"
}

[mspiegel]

Are redirects handled transparently by go-github? I had client.Repositories.DeleteHook() return a 307 response.

[willnorris]

Given that this issue is still open, I guess the answer is... not sure? As I noted in the initial issue description, this should be handled by the http.Client. Maybe it doesn't handle 307 responses? All the more reason for us to add some test cases for this behavior.

[dmitshur]

@huytd, are you still working on this, or should we unassign this issue to free it up for others to work on?

[mspiegel]

The user who has renamed the repository has now deleted the repository so I'm getting a 404. But it should be possible to reproduce the issue by creating a repo and then renaming the repo. I'll try to get a reproducible test case.

[mspiegel]

@jonbodner did some research and the http.client library has hooks for redirecting GET, HEAD, POST, and PUT. But no redirects for DELETE or PATCH. We're not sure yet if this is a bug or a feature in the golang standard library. Jon and I need to try out a couple of different approaches and we'll follow up on this thread.

[jonbodner]

More research: this behavior is changed in Go 1.8 beta. From the release notes:

The Client now supports 307 and 308 redirects. 
If the redirect requires resending the request body, the request must have the new Request.GetBody field defined. 
NewRequest sets Request.GetBody automatically for common body types.

Assuming this code makes it to release, recompiling with Go 1.8 will ensure that DELETE requests that return a 307 status code will redirect automatically.

[kegsay]

Running go 1.7.4 I can confirm that Github returns 307 on a renamed repository and this is not handled by go-github. It looks like waiting for go 1.8 will fix this, but I cannot confirm this yet.

[dmitshur]

Thanks for testing and reporting results, @Kegsay. Which repo did you test on, an do you have a snippet that you've used handy? I have 1.8 here, so I can try and see if it indeed helps.

That said, it wouldn't be sufficient to wait until 1.8 to resolve this, because we currently support versions of Go as old as 1.4.

[kegsay]

@shurcooL I tested it on a repo which was recently renamed, in this case vector-im/vector-web which is now vector-im/riot-web. Attempts to create issues using vector-web fails with the 307. I obviously can't give you a complete example since it's using OAuth tokens, but effectively:

// cli is a *gogithub.Client
issue, res, err := cli.Issues.Create("owner", "repo", &gogithub.IssueRequest{
    Title: "Some title",
    Body:  "Some body",
})
if err != nil {
    fmt.Println("HTTP %d", res.StatusCode)
}

And this will indeed print HTTP 307 (so err and res are non-nil). Renaming repos on Github isn't hard, so you should be able to rig something up to test locally.

[gmlewis]

Closing issue for lack of activity/interest. Feel free to reopen.