Use the Client interface to represent an HTTP client

[francescomari]

Instead of using *http.Client directly, it would be nice if requests used an interface to represent an HTTP client. This allows the user to plug different implementation of the client that implement cross-cutting concerns like retries.

More specifically for me, I would love to use this project to simplify some of my code, but the projects I'm working on already define and pass around interfaces like this to represent an HTTP client:

type HTTPClient interface {
	Do(req *http.Request) (*http.Response, error)
}

Given this prerequisites, it's impossible for me to use this library without type-casting and failing at runtime. Instead, if this PR was merged, integrating with requests would be a piece of cake.

[earthboundkid]

Github is having problems today, so it's eating my comments.

[earthboundkid]

Interesting. It feels sort of redundant with Transport/RoundTripper, but that’s a problem with the standard library (if there’s ever a standard library v2, it should turn http.Client into just a middleware RoundTripper that adds redirect following etc onto a base RoundTripper instead of having two basically identical interfaces with overlapping concerns).

I don't like having setting the Transport fail silently. It should explicitly refuse to connect if given contradictory instructions. The question is whether it should fail by returning an error or panicking. Panicking is more “correct” because setting contradictory options is a programmatic failure, not a runtime failure, but it’s also pretty gross, and I don’t like it.

I think the simpler option that would let us sidestep the question of failing transports is to have a function like this:

// Doer is an interface with a Do method. 
// It exists for compatibility with other libraries.
// Users should prefer Transport, 
// because Do is a wrapper around http.Client 
// which has higher level concerns.
type Doer interface {
	Do(req *http.Request) (*http.Response, error)
}


// DoerTransport converts a Doer into a Transport (http.RoundTripper).
func DoerTransport(cl Doer) Transport {
  return RoundTripFunc(func (req *http.Request) (*http.Response, error) {
    return cl.Do(req)
  })
}

Is there some reason this wouldn’t work with your library?

[francescomari]

The problem with using RoundTripper is that it works at a different level of abstraction than http.Client.

Quoting from the documentation, "RoundTrip executes a single HTTP transaction" (and is therefore the wrong place to plug concerns like retries), and "RoundTrip should not attempt to handle higher-level protocol details such as redirects, authentication, or cookies" (which are all tasks that are anyway already handled via the requests library, because the library acts as an HTTP client with extra features).

Moreover, I should have read the contributing guidelines and started a conversation first, apologies. I just noticed that #56 would be a good match.

I don't like having setting the Transport fail silently. It should explicitly refuse to connect if given contradictory instructions. The question is whether it should fail by returning an error or panicking. Panicking is more “correct” because setting contradictory options is a programmatic failure, not a runtime failure, but it’s also pretty gross, and I don’t like it.

I think there are no good options at this point. I'm in favour for panicking, since setting both a RoundTripper and a custom client that is not an http.Client is a programmer error. But I don't think this will happen often. Why would a user want to plug an HTTP client and a RoundTripper together? In the project mentioned above, we rolled logging, observability and retries in one standard client used all over the place, which makes using RoundTrippers obsolete. The RoundTripper is part of our "standard" HTTP client configuration (indirectly, via go-cleanhttp), and it's a low-level detail I don't usually need to mess with.

[earthboundkid]

I think the docs are a little muddled and out of date. If you read the code for http.Client.Do, all it does is handle retries and apply a CookieJar. It really should just be a middleware RoundTripper, but the Go team is stuck with their old architecture because of the Go 1 guarantee. There are several libraries that handle retries as an http.RoundTripper, such as https://github.com/ybbus/httpretry#modify--customize-the-roundtripper-httptransport, and under the hood, the Google OAuth RoundTripper issues multiple requests (because that's how OAuth works).

[francescomari]

These are all good examples in favour of RoundTrippers as substitute, higher-level HTTP clients. I think your proposal of converting a Doer to a RoundTripper would work in my project, even if it's not as nice as having a generic .Client() method on the builder.

[earthboundkid]

Take a look at v0.23.3-alpha1 and see if it works for your usecase.

[francescomari]

It looks good! Thanks a lot for taking care of this, it will make using this library so much easier for me. I will close this PR, as the change is already in your own.