Go-github: Shouldn't GetArchiveLink accept 301 redirections?

[Soulou]

Currently GetArchiveLink is only accepting 302: https://github.com/google/go-github/blob/0830f2846d799e0c1b07b643f79c4a2af7e54295/github/repos_contents.go#L264

But sometimes, a 301 is returned: I got errors like unexpected status code: 301 Moved Permanently which I assume is related to a renaming of repository.

Shouldn't it be handled?

Thanks a lot

[gmlewis]

Yes, that seems reasonable, to report it as a redirection and then let the client handle trying again with the new URL, or handling it in whatever manner it desires. In other words, I don't think this package should do anything with the information, just report it as a 301.

The GitHub API Developer docs just say: To follow redirects with curl, use the -L switch:.

Anyway, I'm opening up this issue as an opportunity for a contributor to address.

Thanks for the report, @Soulou!

This would be a great PR for any new contributor to this repo or a new Go developer.
All contributions are greatly appreciated!

Feel free to volunteer for any issue and the issue can be assigned to you so that others don't attempt to duplicate the work.

Please check out our CONTRIBUTING.md guide to get started.

Thank you!

[Soulou]

@gmlewis Thanks for your answer, I'll try to have a look if I've time :D

[vaibhavsingh97]

can I take this up?

[Soulou]

Of course if you want it :-)

[vaibhavsingh97]

@Soulou PR sent: https://github.com/google/go-github/pull/1249

[Soulou]

Great thanks.

(gmlewis: The following description is copied from closed PR #1249...)

@gmlewis what I mean is the following:

Without any renaming:

└> curl --user ":$TOKEN" https://api.github.com/repos/user/repo/tarball/master -v -q 2>&1 | grep Location
< Location: https://codeload.github.com/repo/user/legacy.tar.gz/master?token=AACQR4JB4AR5CSOK5KLWXG

The API returns an URL which is directly usable by the client of the package google/go-github, it's authentified by a token valid for 5 minutes (according to GitHub documentation)

Once the app has been renamed, 301 is returned with the following

└> curl --user ":$TOKEN" https://api.github.com/repos/user/renamed/tarball/master -v -q 2>&1 | grep Location
< Location: https://api.github.com/repositories/<ID>/tarball/master

So the user of this package would have to check if:

• It is an URL which can be used to download the tarball itself
• It is still an URL targeting the API which does not contain all the information to make another call again: (indeed we don't have the new name here, and the name is required in this package if I'm not mistaken):

    client.RepositoriesService.GetArchiveLink(
        ctx, owner, repo, github.Tarball,
        &github.RepositoryContentGetOptions{Ref: ref},
    )

I think it would be really a lot of responsability for the user of this package, while if an additional roundtrip would be done here, it would be much easier and logical. GetArchiveLink would always return an archive link as defined in https://developer.github.com/v3/repos/contents/#get-archive-link and not a link to analyze.

[gmlewis]

So let's start over on this one.

Specifically, for the GetArchiveLink endpoint, we would like the ability to detect and follow redirects.

I'm thinking that having the option to follow the redirect might be nice, and have that as the default option. So having a followRedirects=false might be good, then the response could be looked at instead of performing the second roundtrip call.

Before anyone jumps on this issue to implement it, let's first find out what @gauntface, @Soulou, or others think about this idea.

[Soulou]

Having an option is always good for the client, to analyse weird cases/bugs etc, so I agree with it.
To me, the default behaviour should be should be to follow all 301, until a 302 happens and at the moment, returning the Location

[jcockbain]

Would I be able to take a look at this? If so, are we looking for what has been described above? As I've understood it, a followRedirects option on the endpoint which when true follows all 301 responses, until it gets to a 302 and returns a location (an archive link).

[Soulou]

@jcockbain96 please, help yourself, thanks a lot!

(Your implementation idea seems the right one to me, but I'm wondering if it shouldn't be the default and have a dontFollowRedirects)

[jcockbain]

Okay great thanks!

I agree that is should default to following redirects, I'll have a look now as to which way round (follow v dontFollow) fits best with the codebase.

[jcockbain]

Hi all, apologies for the delay in this one. I've got changes on a branch, but have a couple of questions about the implementation.

1. If a user has dontFollowRedirects=true set, and they get a 301 return, what should be returned? At the moment, my code retains the current 301: Status Moved Permanently error. Would this now not be considered an error? Would we want to return the redirect link to the user, or is it up to them to then investigate the link?

2. For testing the dontfollowRedirects=false path, I'm trying to mock the client RoundTrip on a redirect link to give a 302 status code. I'm first mocking a 301 return for the call to /repos/o/r/tarball (which I can do using mux.handleFunc()), with a link that will give a 302 return from client.RoundTrip (and an archive link). This test is on my branch, under a TODO. Is anyone able to advise on whether this is the right approach, and if there's existing code to mock the RoundTrip?

Git diff: https://github.com/google/go-github/compare/master...jcockbain:follow-redirects-archive-link

[gmlewis]

These are my opinions, and I'm totally open to discussion...

First, naming the flag dontFollowRedirects=<bool> is just plain confusing. My brain has to do all sorts of mental gymnastics to figure out the single or double negative. So my first recommendation is to name the option followRedirects, and I think it is fine to force the client to explicitly set this to true, since the historical behavior has been to _not_ follow redirects, so the default behavior is to behave as it used to behave.

So, to answer question 1 above, I would say that for followRedirects=false, the behavior is identical to the current behavior, which I believe is to return the error.

As for question 2 above, have you taken a look at https://golang.org/pkg/net/http/httptest/ and https://golang.org/pkg/net/http/httputil/ ? These are super-handy packages for mocking and testing real live (internal) connections that really use the http protocol, and I highly recommend them.