code review 10234052: google-api-go-client: set opaque correctly in pre go1.1...

google-api-go-client: set opaque correctly in pre go1.1 versions

[francesc, 2013-06-14 00:54:16]

Hello adg@golang.org, bradfitz@golang.org (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/google-api-go-client

[bradfitz, 2013-06-14 08:43:11]

https://codereview.appspot.com/10234052/diff/14001/googleapi/googleapi.go
File googleapi/googleapi.go (right):

https://codereview.appspot.com/10234052/diff/14001/googleapi/googleapi.go#new...
googleapi/googleapi.go:243: var isGo11 = true
I think this is more readable if this is the other way around.  Make this be:

// isGo11 is set true by go11.go when we're at Go1.1 or later.
var isGo11 bool

Then rename prego11.go to just go11.go and set this to true there.

https://codereview.appspot.com/10234052/diff/14001/googleapi/googleapi.go#new...
googleapi/googleapi.go:245: // IsGo11 returns true if the current compiler is
go1.1 or newer.
doc comment is about the wrong thing.

I'm not sure what this is trying to do.

https://codereview.appspot.com/10234052/diff/14001/googleapi/googleapi.go#new...
googleapi/googleapi.go:249: u.Opaque = u.Scheme + ":" + u.Opaque
isn't u.Opaque typically just empty here? when would it be non-empty at the
point where callers called this?

https://codereview.appspot.com/10234052/diff/14001/googleapi/googleapi_test.go
File googleapi/googleapi_test.go (right):

https://codereview.appspot.com/10234052/diff/14001/googleapi/googleapi_test.g...
googleapi/googleapi_test.go:8: func TestSetOpaque(t *testing.T) {
I think you need more than one test.

What about all the crazy cases from before with slashes and %2F and stuff?

I want tests to see how the http.Request.Write() writes.  Write to a
bytes.Buffer and compare the valid output(s)?

[francesc, 2013-06-14 23:02:45]

PTAL

https://codereview.appspot.com/10234052/diff/14001/googleapi/googleapi.go
File googleapi/googleapi.go (right):

https://codereview.appspot.com/10234052/diff/14001/googleapi/googleapi.go#new...
googleapi/googleapi.go:243: var isGo11 = true
On 2013/06/14 08:43:11, bradfitz wrote:
> I think this is more readable if this is the other way around.  Make this be:
> 
> // isGo11 is set true by go11.go when we're at Go1.1 or later.
> var isGo11 bool
> 
> Then rename prego11.go to just go11.go and set this to true there.

Done.

https://codereview.appspot.com/10234052/diff/14001/googleapi/googleapi.go#new...
googleapi/googleapi.go:245: // IsGo11 returns true if the current compiler is
go1.1 or newer.
On 2013/06/14 08:43:11, bradfitz wrote:
> doc comment is about the wrong thing.
> 
> I'm not sure what this is trying to do.

Done.

https://codereview.appspot.com/10234052/diff/14001/googleapi/googleapi.go#new...
googleapi/googleapi.go:249: u.Opaque = u.Scheme + ":" + u.Opaque
On 2013/06/14 08:43:11, bradfitz wrote:
> isn't u.Opaque typically just empty here? when would it be non-empty at the
> point where callers called this?

Opaque is not empty at this point, since I modify it right before.

The point of this is to avoid having "%2F" encoded back to "/" in the request.

https://codereview.appspot.com/10234052/diff/14001/googleapi/googleapi_test.go
File googleapi/googleapi_test.go (right):

https://codereview.appspot.com/10234052/diff/14001/googleapi/googleapi_test.g...
googleapi/googleapi_test.go:8: func TestSetOpaque(t *testing.T) {
On 2013/06/14 08:43:11, bradfitz wrote:
> I think you need more than one test.
> 
> What about all the crazy cases from before with slashes and %2F and stuff?
> 
> I want tests to see how the http.Request.Write() writes.  Write to a
> bytes.Buffer and compare the valid output(s)?

Done.

[bradfitz, 2013-06-18 03:12:37]

Let me know when you've run these tests on both Go 1.0 and Go 1.1, and the
problematic API(s) work on both too.

https://codereview.appspot.com/10234052/diff/20001/googleapi/googleapi.go
File googleapi/googleapi.go (right):

https://codereview.appspot.com/10234052/diff/20001/googleapi/googleapi.go#new...
googleapi/googleapi.go:245: // SetOpaque sets Opaque in the passed URL so
RequestURI returns a correct
Maybe:

// SetOpaque sets u.Opaque from u.Path such that HTTP requests to it
// don't alter any hex-escaped characters in u.Path.

https://codereview.appspot.com/10234052/diff/20001/googleapi/googleapi.go#new...
googleapi/googleapi.go:246: // URI avoiding decoding of hex escaped characters.
hex-escaped

https://codereview.appspot.com/10234052/diff/20001/googleapi/googleapi_test.go
File googleapi/googleapi_test.go (right):

https://codereview.appspot.com/10234052/diff/20001/googleapi/googleapi_test.g...
googleapi/googleapi_test.go:16: const reqTmpl = "GET %v HTTP/1.1\r\nHost:
%v\r\nUser-Agent: Go 1.1 package http\r\n\r\n"
You didn't test this against Go 1.0.3 before mailing, since this template
contains "Go 1.1 package http"

https://codereview.appspot.com/10234052/diff/20001/googleapi/googleapi_test.g...
googleapi/googleapi_test.go:20: out string
out is a weird name now. (confusing because it's an input to a function which
makes the desired output) wantRequestURI is long, but accurate, and you're not
using tagged struct literals anyway.

https://codereview.appspot.com/10234052/diff/20001/googleapi/googleapi_test.g...
googleapi/googleapi_test.go:88: q := &http.Request{URL: &u}
s/q/req/ ? or r?

[campoy, 2013-06-18 18:30:18]

https://codereview.appspot.com/10234052/diff/20001/googleapi/googleapi.go
File googleapi/googleapi.go (right):

https://codereview.appspot.com/10234052/diff/20001/googleapi/googleapi.go#new...
googleapi/googleapi.go:245: // SetOpaque sets Opaque in the passed URL so
RequestURI returns a correct
On 2013/06/18 03:12:38, bradfitz wrote:
> Maybe:
> 
> // SetOpaque sets u.Opaque from u.Path such that HTTP requests to it
> // don't alter any hex-escaped characters in u.Path.

Done.

https://codereview.appspot.com/10234052/diff/20001/googleapi/googleapi.go#new...
googleapi/googleapi.go:246: // URI avoiding decoding of hex escaped characters.
On 2013/06/18 03:12:38, bradfitz wrote:
> hex-escaped

Done.

https://codereview.appspot.com/10234052/diff/20001/googleapi/googleapi_test.go
File googleapi/googleapi_test.go (right):

https://codereview.appspot.com/10234052/diff/20001/googleapi/googleapi_test.g...
googleapi/googleapi_test.go:16: const reqTmpl = "GET %v HTTP/1.1\r\nHost:
%v\r\nUser-Agent: Go 1.1 package http\r\n\r\n"
I did run it, but the GOROOT wasn't correctly set, sorry.

Now fixed and tested with both versions on a short program using %2F
communicating with Cloud Storage and it works perfectly.

https://codereview.appspot.com/10234052/diff/20001/googleapi/googleapi_test.g...
googleapi/googleapi_test.go:20: out string
On 2013/06/18 03:12:38, bradfitz wrote:
> out is a weird name now. (confusing because it's an input to a function which
> makes the desired output) wantRequestURI is long, but accurate, and you're not
> using tagged struct literals anyway.

Done.

https://codereview.appspot.com/10234052/diff/20001/googleapi/googleapi_test.g...
googleapi/googleapi_test.go:88: q := &http.Request{URL: &u}
On 2013/06/18 03:12:38, bradfitz wrote:
> s/q/req/ ? or r?

Done.

[bradfitz, 2013-06-18 19:27:46]

LGTM

Submit this first, and then we can do the auto-generated files in a
separate CL.

Thanks!



On Tue, Jun 18, 2013 at 11:30 AM, <campoy@google.com> wrote:

>
> https://codereview.appspot.**com/10234052/diff/20001/**
>
googleapi/googleapi.go<https://codereview.appspot.com/10234052/diff/20001/googleapi/googleapi.go>
> File googleapi/googleapi.go (right):
>
> https://codereview.appspot.**com/10234052/diff/20001/**
>
googleapi/googleapi.go#**newcode245<https://codereview.appspot.com/10234052/diff/20001/googleapi/googleapi.go#newcode245>
> googleapi/googleapi.go:245: // SetOpaque sets Opaque in the passed URL
> so RequestURI returns a correct
> On 2013/06/18 03:12:38, bradfitz wrote:
>
>> Maybe:
>>
>
>  // SetOpaque sets u.Opaque from u.Path such that HTTP requests to it
>> // don't alter any hex-escaped characters in u.Path.
>>
>
> Done.
>
>
> https://codereview.appspot.**com/10234052/diff/20001/**
>
googleapi/googleapi.go#**newcode246<https://codereview.appspot.com/10234052/diff/20001/googleapi/googleapi.go#newcode246>
> googleapi/googleapi.go:246: // URI avoiding decoding of hex escaped
> characters.
> On 2013/06/18 03:12:38, bradfitz wrote:
>
>> hex-escaped
>>
>
> Done.
>
>
> https://codereview.appspot.**com/10234052/diff/20001/**
>
googleapi/googleapi_test.go<https://codereview.appspot.com/10234052/diff/20001/googleapi/googleapi_test.go>
> File googleapi/googleapi_test.go (right):
>
> https://codereview.appspot.**com/10234052/diff/20001/**
>
googleapi/googleapi_test.go#**newcode16<https://codereview.appspot.com/10234052/diff/20001/googleapi/googleapi_test.go#newcode16>
> googleapi/googleapi_test.go:**16: const reqTmpl = "GET %v
> HTTP/1.1\r\nHost: %v\r\nUser-Agent: Go 1.1 package http\r\n\r\n"
> I did run it, but the GOROOT wasn't correctly set, sorry.
>
> Now fixed and tested with both versions on a short program using %2F
> communicating with Cloud Storage and it works perfectly.
>
>
> https://codereview.appspot.**com/10234052/diff/20001/**
>
googleapi/googleapi_test.go#**newcode20<https://codereview.appspot.com/10234052/diff/20001/googleapi/googleapi_test.go#newcode20>
> googleapi/googleapi_test.go:**20: out string
> On 2013/06/18 03:12:38, bradfitz wrote:
>
>> out is a weird name now. (confusing because it's an input to a
>>
> function which
>
>> makes the desired output) wantRequestURI is long, but accurate, and
>>
> you're not
>
>> using tagged struct literals anyway.
>>
>
> Done.
>
>
> https://codereview.appspot.**com/10234052/diff/20001/**
>
googleapi/googleapi_test.go#**newcode88<https://codereview.appspot.com/10234052/diff/20001/googleapi/googleapi_test.go#newcode88>
> googleapi/googleapi_test.go:**88: q := &http.Request{URL: &u}
> On 2013/06/18 03:12:38, bradfitz wrote:
>
>> s/q/req/ ? or r?
>>
>
> Done.
>
>
https://codereview.appspot.**com/10234052/<https://codereview.appspot.com/102...
>

[francesc, 2013-06-18 19:56:10]

*** Submitted as
https://code.google.com/p/google-api-go-client/source/detail?r=1daa95ee8e04 ***

google-api-go-client: set opaque correctly in pre go1.1 versions

R=adg, bradfitz, campoy
CC=golang-dev
https://codereview.appspot.com/10234052