Add client.BareDo to allow user to handle the body

[azr]

This allows the user to also treat bodies as plain go streams.
For more flexibility.

Before this, passing a nil body would do the request but close the
body. Now it is up to the caller to close resp.Body.

I mainly wanted to be able to do this to stream bodies and to re-use the same HTTP client in order to download release files.

Edit:
This PR enables using the client as an http client with Github Specific error handling and throttling management.

[codecov]

Codecov Report

Merging #1772 (6d84976) into master (7081b5f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1772   +/-   ##
=======================================
  Coverage   97.53%   97.53%           
=======================================
  Files          98       98           
  Lines        6373     6382    +9     
=======================================
+ Hits         6216     6225    +9     
  Misses         85       85           
  Partials       72       72           

Impacted Files	Coverage Δ
github/github.go	96.61% <100.00%> (+0.05%)	⬆️
github/repos.go	98.72% <0.00%> (ø)
github/repos_commits.go	100.00% <0.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7081b5f...6d84976. Read the comment docs.

[gmlewis]

Hmmm... I'm concerned that this change might be too disruptive for users of this package. If I understand correctly, it would force all users to add code to manually close the body.

[willnorris]

I had the same thought... this seems like a pretty risky change.

[azr]

Yes, that is correct ! Is that an issue even for v34 ?

Edit: To be precise it would force users that do nothing with the response's body. Also I would totally understand if this was non mergeable. Turn out its better to have a different client for downloading release files.

[gmlewis]

Yes, that is correct ! Is that an issue even for v34 ?

Yes. While bumping version numbers is indeed used for breaking API changes, this change I feel is too disruptive for the majority of users of this package to be generally useful.

I'm happy to leave this issue/PR open for discussion, but if there is not much interest in it, I think we will most likely close it.

Alternatively, if you could come up with a PR that would default to existing behavior but as an option people could enable your proposed new behavior, that might be more palatable for inclusion in this repo.

[azr]

Hey @gmlewis,

this change I feel is too disruptive for the majority of users of this package to be generally useful.

Gotcha, I understand 🙂 . The api doesn't change so unless you read the changelog... Its an easy trap.

but if there is not much interest in it, I think we will most likely close it.

After some thinking I still would like to reuse the same client to download binaries from Github. In case they start adding api limitations or something. So I still would like to make this work. Also I like to handle the body like if I was doing a plain http request.

I was thinking of adding a new function that looks like so:

// ___ Is just like Do but will let you handle the body.
// Make sure to close the Response Body.
func (c *Client) ___(ctx context.Context, req *http.Request) (*Response, error) {

In terms of name I was thinking of something like BareDo ? DoWithContext ( meh ).

I'm also happy to implement something with your idea:

So allowing to call:

client.Do(ctx, req, github.LeaveBody)

Ah naming is hard :D Do you have a suggestion there ?

( I like my suggestion to add a new BareDo function a tad more, because it feels simple and sort of godoc friendly, I will implement it to gain some time but feel free to make me change it ).

[azr]

Hello there, commenting here just to say that this works for me. I had a tiny issue where the auth token from github was being set again to a request done to aws which was 400'ing because it wants only one auth token, I was able to fix this using the following code:

fuunc something() {
	ts := oauth2.StaticTokenSource(
		&oauth2.Token{AccessToken: tk},
	)
	tc = &http.Client{
		Transport: &HostSpecificTokenAuthTransport{
			TokenSources: map[string]oauth2.TokenSource{
				"api.github.com": ts,
			},
		},
	}
}

type HostSpecificTokenAuthTransport struct {
        // Host to TokenSource map
	TokenSources map[string]oauth2.TokenSource

	// Transport is the underlying HTTP transport to use when making requests.
	// It will default to http.DefaultTransport if nil.
	Transport http.RoundTripper

	Base http.RoundTripper
}

// RoundTrip authorizes and authenticates the request with an
// access token from Transport's Source.
func (t *HostSpecificTokenAuthTransport) RoundTrip(req *http.Request) (*http.Response, error) {
	source, found := t.TokenSources[req.Host]
	if found {
		reqBodyClosed := false
		if req.Body != nil {
			defer func() {
				if !reqBodyClosed {
					req.Body.Close()
				}
			}()
		}

		if source == nil {
			return nil, errors.New("transport's Source is nil")
		}
		token, err := source.Token()
		if err != nil {
			return nil, err
		}

		token.SetAuthHeader(req)

		// req.Body is assumed to be closed by the base RoundTripper.
		reqBodyClosed = true
	}

	return t.base().RoundTrip(req)
}


func (t *HostSpecificTokenAuthTransport) base() http.RoundTripper {
	if t.Base != nil {
		return t.Base
	}
	return http.DefaultTransport
}

This seems to be related with #246 would you like me to contribute this Transport here ? It could also very well reside in the oauth package.

[gmlewis]

I'm not sure I fully understand how you expect BareDo to be used since most (all?) endpoints in this repo call Do directly.

Do you expect users of this library to call it directly from their own code?

[azr]

That's what I'd like to do, yes. I need all the nice error handling and throttling management but would like to handle the bodies myself, just like if I was using an plain http.Client that knows a little more about the Github API. It makes this library even more powerful to me. Because it'd be sort of replaceable.

[gmlewis]

Overall, I'm liking this change and like the switch cleanup as well.
I added a couple comments/questions if you could please address them.

Thank you, @azr !

[azr]

Okay I updated the PR Thanks for reviewing 🙂

[gmlewis]

One more thought... might it be a good idea to add a unit test for BareDo that demonstrates how the body is not yet closed?

[azr]

Good idea, I added a func to test this 🙂

[gmlewis]

Thank you, @azr !
One tiny tweak, please, otherwise LGTM.

After fixing the typo, awaiting second LGTM before merging.

[gmlewis]

Thank you, @azr !
LGTM.

Awaiting second LGTM before merging.

[wesleimp]

LGTM 👌🏼

[gmlewis]

Thank you, @wesleimp !
Merging.

[azr]

Nice ! :D thanks for your time !