RepositoriesService.DownloadContents should handle response codes

[mctofu]

To download the contents of a file the DownloadContents method is using a http.Client to request it and then returns only the body of the response.

			resp, err := s.client.client.Get(*contents.DownloadURL)
			if err != nil {
				return nil, err
			}
			return resp.Body, nil

The http.Client docs state that A non-2xx response doesn't cause an error. If the download request returns a failed response code then resp.Body most likely doesn't contain the content you wanted and there is no way to tell that something went wrong.

I'd suggest using github.CheckResponse on the response or perhaps a simpler 2XX check since raw.githubusercontent.com may have different behavior than the api.

[gmlewis]

Thanks for the report, @mctofu.

I'm thinking that using github.CheckResponse is probably not going to be a good option here due to the overhead of it slurping in an entire copy of the body (which could be upwards of 50MB).

Possibly a simpler approach would just be to additionally return the response itself like most of the other endpoints in this repo do. That way the client can check it directly. This would obviously be a breaking API change, but we already have two breaking API changes in the queue for the next release.

So instead of return resp.Body, nil, we would have return resp.Body, resp, nil.

Thoughts?

[mctofu]

Yeah, I think it could make sense to also return the response since that would be consistent with the other endpoints. What's interesting is that DownloadContents is more of a convenience helper that makes multiple calls so could the returned response actually be from GetContents if that returned an error?

Being able to check the status code on the response myself would solve this issue. It's not as consistent with other endpoints which would return an error on a bad status code though. That might make it easier to miss that the response code should be checked manually.

[mctofu]

Maybe return resp, nil would be a better option? Then it could be a little more clear that you are getting the raw response and should be checking the status code?

[gmlewis]

Possibly.

I do like the return of the io.ReadCloser to make it clear that clients really need to read it and then close it... so I would lean toward the return resp.Body, resp, nil solution. Also, the three return values also is highly consistent with the rest of the repo... so that is my vote. Feel free to let this issue stay open and see if anyone else has opinions on how to address this issue and we can get a discussion going if so.

[mctofu]

Ok! Any option that allows detecting a bad response is ok with me. I was hoping to save others from running into this same issue since it was pretty tricky to track down. Making it clear in the docs that you should check the response code would be appreciated!

[gmlewis]

So I think a PR needs to be written that exposes the response code to the end user.
@mctofu - are you interested in owning this issue and submitting a PR, or would you like me to open it up for other volunteers to address?

[mctofu]

@gmlewis: I could submit a PR within the next week (hopefully sooner) but if someone else was interested in tackling this that would be fine by me as well.

[gmlewis]

OK, then I'll just leave this open, and if you or anyone else reports that they are starting work on it, I'll assign it to them. Thank you, @mctofu !