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

RoundTrip: avoid modifying the original request #805

Merged
merged 4 commits into from Dec 21, 2017

Conversation

Projects
None yet
4 participants
@sahildua2305
Collaborator

sahildua2305 commented Dec 6, 2017

Avoid modifying the original request as per http.RoundTripper contract.
In UnauthenticatedRateLimitedTransport.RoundTrip, we need to modify the
URL of the request only, while in BasicAuthTransport.RoundTrip, we need
to modify only the headers.

Hence, we get rid of cloneRequest method which wasn't working good for
the needs of UnauthenticatedRateLimitedTransport.RoundTrip. Instead, now
we have the implementation of cloneRequest inline in both the RoundTrip
interfaces.

We decided to make the cloneRequest implementation inline because of its
use at only these two places and we think we won't gain much by making a
generic implementation of cloneRequest as a function. To follow the
discussion on this, check the GitHub issue - #556.

Fixes #556

RoundTrip: avoid modifying the original request
Avoid modifying the original request as per http.RoundTripper contract.
In UnauthenticatedRateLimitedTransport.RoundTrip, we need to modify the
URL of the request only, while in BasicAuthTransport.RoundTrip, we need
to modify only the headers.

Hence, we get rid of cloneRequest method which wasn't working good for
the needs of UnauthenticatedRateLimitedTransport.RoundTrip. Instead, now
we have the implementation of cloneRequest inline in both the RoundTrip
interfaces.

We decided to make the cloneRequest implementation inline because of its
use at only these two places and we think we won't gain much by making a
generic implementation of cloneRequest as a function. To follow the
discussion on this, check the GitHub issue -
#556.

Fixes #556

@googlebot googlebot added the cla: yes label Dec 6, 2017

@sahildua2305

This comment has been minimized.

Show comment
Hide comment
@sahildua2305

sahildua2305 Dec 6, 2017

Collaborator

@shurcooL I am struggling with tests for this change. I tried this -

tp := &UnauthenticatedRateLimitedTransport{
    ClientID: "id",
    ClientSecret: "secret",
}
req := httptest.NewRequest("GET", "/", nil)
want := req.URL.Path
_, err := tp.RoundTrip(req)
if err != nil {
    t.Errorf("Did not expect RoundTrip to return error")
}
if got := req.URL.Path; got != want {
    t.Errorf("RoundTrip should not modify the original request")
}

However, that doesn't work because RoundTrip request itself fails.

Collaborator

sahildua2305 commented Dec 6, 2017

@shurcooL I am struggling with tests for this change. I tried this -

tp := &UnauthenticatedRateLimitedTransport{
    ClientID: "id",
    ClientSecret: "secret",
}
req := httptest.NewRequest("GET", "/", nil)
want := req.URL.Path
_, err := tp.RoundTrip(req)
if err != nil {
    t.Errorf("Did not expect RoundTrip to return error")
}
if got := req.URL.Path; got != want {
    t.Errorf("RoundTrip should not modify the original request")
}

However, that doesn't work because RoundTrip request itself fails.

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Dec 6, 2017

Member

I am struggling with tests for this change.

I don't think this change needs a test. We'll just review it carefully to ensure that the URL field of original request is not being modified, and the existing package tests will ensure that overall functionality is good.

Member

dmitshur commented Dec 6, 2017

I am struggling with tests for this change.

I don't think this change needs a test. We'll just review it carefully to ensure that the URL field of original request is not being modified, and the existing package tests will ensure that overall functionality is good.

@sahildua2305

This comment has been minimized.

Show comment
Hide comment
@sahildua2305

sahildua2305 Dec 6, 2017

Collaborator

I don't think this change needs a test.

Okay, then this PR is ready for review.

Collaborator

sahildua2305 commented Dec 6, 2017

I don't think this change needs a test.

Okay, then this PR is ready for review.

@dmitshur dmitshur self-requested a review Dec 6, 2017

@sahildua2305

This comment has been minimized.

Show comment
Hide comment
@sahildua2305

sahildua2305 Dec 14, 2017

Collaborator

@shurcooL did you have a chance to look at this? I know this is a low priority pull request. Just a gentle reminder.

Collaborator

sahildua2305 commented Dec 14, 2017

@shurcooL did you have a chance to look at this? I know this is a low priority pull request. Just a gentle reminder.

@dmitshur

Thanks for implementing a fix for this @sahildua2305.

Unfortunately, it's not looking as readable as I was hoping it would, but I'm not able to come up with any better alternatives even after spending some time trying. I think it's acceptable in the end (and the http.RoundTripper contract is to blame for this).

Everything looks correct to me. Two minor suggestions, otherwise LGTM. But I'll ask for a second review just in case I missed something.

Show outdated Hide outdated github/github.go
Show outdated Hide outdated github/github.go

@dmitshur dmitshur requested a review from gmlewis Dec 14, 2017

@gmlewis

One question out of curiosity, but otherwise LGTM.
Thank you, @sahildua2305 and @shurcooL!

*req2 = *req
req2.Header = make(http.Header, len(req.Header))
for k, s := range req.Header {
req2.Header[k] = append([]string(nil), s...)

This comment has been minimized.

@gmlewis

gmlewis Dec 20, 2017

Collaborator

Why not []string{} instead of []string(nil)?
I see it was written that way before in cloneRequest but I'm not sure I understand the difference.
[]string{} seems more common (to me) but maybe it's not a big deal?

@gmlewis

gmlewis Dec 20, 2017

Collaborator

Why not []string{} instead of []string(nil)?
I see it was written that way before in cloneRequest but I'm not sure I understand the difference.
[]string{} seems more common (to me) but maybe it's not a big deal?

This comment has been minimized.

@sahildua2305

sahildua2305 Dec 20, 2017

Collaborator

@gmlewis good one! I didn't think about it. Definitely, []string{} is more common one. I'll change that in next commit. Thanks for reviewing!

@sahildua2305

sahildua2305 Dec 20, 2017

Collaborator

@gmlewis good one! I didn't think about it. Definitely, []string{} is more common one. I'll change that in next commit. Thanks for reviewing!

This comment has been minimized.

@dmitshur

dmitshur Dec 20, 2017

Member

Why not []string{} instead of []string(nil)?

@gmlewis I think it's because that's the style used in Go slice tricks document, where this "copy slice" pattern is taken from. I think it's better to stay consistent with that.

If you think it should be changed, I suggest you make the proposal to change Go slice tricks, then we can update our style here.

Logical behavior should be the same, but without testing, I can't be sure if append([]string{}, s...) causes an extra allocation or not compared to append([]string(nil), s...). (Hopefully/probably it's the same.)

@dmitshur

dmitshur Dec 20, 2017

Member

Why not []string{} instead of []string(nil)?

@gmlewis I think it's because that's the style used in Go slice tricks document, where this "copy slice" pattern is taken from. I think it's better to stay consistent with that.

If you think it should be changed, I suggest you make the proposal to change Go slice tricks, then we can update our style here.

Logical behavior should be the same, but without testing, I can't be sure if append([]string{}, s...) causes an extra allocation or not compared to append([]string(nil), s...). (Hopefully/probably it's the same.)

This comment has been minimized.

@gmlewis

gmlewis Dec 20, 2017

Collaborator

Oh, weird! Thanks for the pointer, @shurcooL. OK, I'm fine with keeping it that way. I just thought it was odd-looking... looks odd there too, but that's fine... no need to change it. I'll shut up now. 😄

@gmlewis

gmlewis Dec 20, 2017

Collaborator

Oh, weird! Thanks for the pointer, @shurcooL. OK, I'm fine with keeping it that way. I just thought it was odd-looking... looks odd there too, but that's fine... no need to change it. I'll shut up now. 😄

This comment has been minimized.

@sahildua2305

sahildua2305 Dec 20, 2017

Collaborator

Thanks for the pointer, @shurcooL. Changed it back 😄

@sahildua2305

sahildua2305 Dec 20, 2017

Collaborator

Thanks for the pointer, @shurcooL. Changed it back 😄

RoundTrip: minor fix
Change []string(nil) to []string{} to make it easier to understand
because the former isn't a common pattern in Go.
@gmlewis

LGTM.

@dmitshur dmitshur merged commit 1acce21 into google:master Dec 21, 2017

2 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur
Member

dmitshur commented Dec 21, 2017

Thank you @sahildua2305 and @gmlewis!

nbareil pushed a commit to nbareil/go-github that referenced this pull request May 1, 2018

Avoid modifying original request in RoundTrip. (google#805)
Avoid modifying the original request as per http.RoundTripper contract.
In UnauthenticatedRateLimitedTransport.RoundTrip, we need to modify the
URL of the request only, while in BasicAuthTransport.RoundTrip, we need
to modify only the headers.

We get rid of cloneRequest helper, which wasn't working well for the needs
of UnauthenticatedRateLimitedTransport.RoundTrip. Instead, now we have
the implementation of cloneRequest inlined in both RoundTrip methods.

We decided to make the cloneRequest implementation inlined because its used
at only these two places, and we think we won't gain much by making a
generic implementation of cloneRequest as a function. For more information,
see issue google#556.

Fixes google#556.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment