search

f2prateek / train

Chainable HTTP client middleware.
http://f2prateek.com/2016/09/10/powering-up-http-clients-with-train/
MIT License
31 stars 4 forks source link

[Question] should Train let you modify a request inside an Interceptor? #12

Open frojasg opened 7 years ago

frojasg commented 7 years ago

Hi,

The RoundTripper documentation says:

RoundTrip should not modify the request, except for consuming and closing the Request's Body.

The example in the README.md is mutating the request which goes against the library documentation.

If you modify a request inside an interceptor, wouldn't it lead to unexpected behaviors?

I tried to dig a little bit deeper why that comment was there, but I couldn't find an answer. This CL is when the comment was introduced to solve this issue, but it doesn't explain why :(.

P.S.: I loved this project, it's a pretty cool idea.

f2prateek commented 7 years ago

Interesting. I hadn't seen that note, nor have we seen adverse effects of doing so.

From here https://codereview.appspot.com/5284041/patch/1002/4, it looks it internally does mutate the request (though it undos it as well). So while I think it's meant to serve as a future warning, it should be ok given the current implementation.

frojasg commented 7 years ago

Thank you!. Feel free to close this issue if you want 😄

frojasg commented 7 years ago

I think this thread is relevant https://groups.google.com/forum/#!searchin/golang- nuts/RoundTripper%7Csort:relevance/golang-nuts/U4FPkQKcAzM/xPireKYNvowJ

f2prateek commented 7 years ago

The RoundTripper simply does not own the request, nor the header.

Yeah I think that's the key part. We can certainly improve the example to make it "proper" by copying the request, etc, but I think the act of sneaking in data via the request header should be ok.

One thing we could do is make chain.Request() return a copy of the request. Or add some convencience functions for it.

mrsinham commented 6 years ago

Hello, a year late but as the oauth2 library modify the request (by copying the request), the train library should handle request cancellation as the request can be changed. Indeed, the sub roundtripper can create a new request based on the top one. But if the top one is cancelled, the new one must be too. Maybe it can be a helper for roundtripper but this is one purpose of chaining roundtripper, altering the requests (ie: to do auth layer)