Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unable to set custom http.RoundTripper #65

Closed
bak1an opened this issue Mar 30, 2017 · 10 comments
Closed

Unable to set custom http.RoundTripper #65

bak1an opened this issue Mar 30, 2017 · 10 comments
Assignees

Comments

@bak1an
Copy link
Contributor

bak1an commented Mar 30, 2017

At the moment it is not possible to set custom http.RoundTripper as a transport in resty since its transport field wants exactly http.Transport struct. On the contrary, golang's stdlib client allows us to use any RoundTripper - https://golang.org/src/net/http/client.go?s=1933:4055#L46

Providing custom RoundTripper can be usable in testing, i.e. one could hijack transport to be provided by some mocking library (https://github.com/dnaeon/go-vcr) or when there is a need to use custom transport for whatever reasons like connecting to weird places with weird protocols or using transport with more features than golang's one provides (https://github.com/pkulak/simpletransport).

Resty is currently relying on http.Transport to provide a way for controlling proxies, tls and timeouts.

We could use type assertion in those places to check if RoundTripper we have is an http.Transport and do nothing in case it is not (or returning error). We could also mention it in the documentation that those things are controlled by underlying transport so SetProxy, SetTimeout and SetTLSClientConfig won't function in case custom non http.Transport transport used.

I can provide a pull request if you find the idea to be valid.

@jeevatkm
Copy link
Member

@bak1an I need a time to analyze and get back to you. However your description makes sense.

@jeevatkm jeevatkm self-assigned this Apr 5, 2017
@jeevatkm
Copy link
Member

jeevatkm commented Apr 18, 2017

@bak1an I took longer than I expected to respond for your query.

I would like to give you a background. Since v0.11 release -

  • After calling SetTransport method and then user calls any methods, those supplied values (Proxy, TLS, etc.) gets into the object they have supplied and resty is using that internaly.
  • Timeout is applied in the http.Client.Timeout

Now let's get back to your question, whether adding support for http.RoundTripper and keeping existing features intact would benefit resty users? If answer is yes then please count me in.

Looking forward to your thoughts.

@bak1an
Copy link
Contributor Author

bak1an commented Apr 24, 2017

Let me take another look at this...

@jeevatkm
Copy link
Member

Sure, I will wait for your inputs.

@jeevatkm
Copy link
Member

jeevatkm commented May 4, 2017

Whenever you have inputs please let me know. Thanks.

@bak1an
Copy link
Contributor Author

bak1an commented May 8, 2017

@jeevatkm

Existing timeout, TLS and proxy resty interfaces are looking simple and solid so we definitely should keep them around without making anything there harder to use.

On the other hand, giving users ability to override RoundTripper will provide the library with extra flexibility which is good thing to have as well.

The way to get this done I see at the moment is to provide users with SetRoundTripper method on Client along with documentation describing that tls/timeout/proxy methods will not work unless that RoundTripper provided is not an http.Transport. So it is more like "if you know what you are doing - have fun" kind of method.

And the proxy/timeout/tls methods stay with the same interface, they will just do type assertion on roundTripper they have available and do nothing if it is not and http.Transport (+ log the error like in https://github.com/go-resty/resty/blob/master/client.go#L535 to get user and idea that something is likely wrong).

Does this sound sane?

@jeevatkm
Copy link
Member

jeevatkm commented May 8, 2017

@bak1an

Yes it's sounds good. Thank you.

@bak1an
Copy link
Contributor Author

bak1an commented May 8, 2017

@jeevatkm Then I will make a PR for this around the end of the week. Thanks!

@jeevatkm
Copy link
Member

@bak1an working on PR #74 for this feature.

@jeevatkm
Copy link
Member

Implementation is in #91, closing it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants