code review 9671043: google-api-go-client: avoid reencoding parameters in th...

google-api-go-client: avoid reencoding parameters in the URL path

Some APIs, such as cloud storage, need to use values that are being
now removed.
'%2F' is replaced by '/' url.Parse, which is called by http.NewRequest.

[francesc, 2013-05-22 19:58:37]

Hello bradfitz@golang.org, adg@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-05-22 20:08:07]

does this break other APIs, though?

I'm afraid you're only testing cloud storage.



On Wed, May 22, 2013 at 12:58 PM, <campoy@golang.org> wrote:

> Reviewers: bradfitz, adg,
>
> Message:
> Hello bradfitz@golang.org, adg@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<https://code.google.com/p/go...
>
>
> Description:
> google-api-go-client: avoid reencoding parameters in the URL path
>
> Some APIs, such as cloud storage, need to use values that are being
> now removed.
> '%2F' is replaced by '/' url.Parse, which is called by http.NewRequest.
>
> Please review this at
https://codereview.appspot.**com/9671043/<https://codereview.appspot.com/9671...
>
> Affected files:
>   M google-api-go-generator/gen.go
>
>
> Index: google-api-go-generator/gen.go
> ==============================**==============================**=======
> --- a/google-api-go-generator/gen.**go
> +++ b/google-api-go-generator/gen.**go
> @@ -488,7 +488,6 @@
>                 res.generateMethods()
>         }
>
> -       pn("\nfunc cleanPathString(s string) string { return
> strings.Map(func(r rune) rune { if r >= 0x2d && r <= 0x7a || r == '~' {
> return r }; return -1 }, s) }")
>         return nil
>  }
>
> @@ -1137,9 +1136,6 @@
>                 pn(`params.Set("uploadType", "multipart")`)
>                 pn("}")
>         }
> -       for _, arg := range args.forLocation("path") {
> -               p("\turls = strings.Replace(urls, \"{%s}\", %s, 1)\n",
> arg.apiname, arg.cleanExpr("c."))
> -       }
>         pn("urls += \"?\" + params.Encode()")
>         if meth.supportsMedia() {
>                 if !hasContentType { // Support mediaUpload but no ctype
> set.
> @@ -1150,6 +1146,14 @@
>                 pn("contentLength_, hasMedia_ := googleapi.**
> ConditionallyIncludeMedia(c.**media_, &body, &ctype)")
>         }
>         pn("req, _ := http.NewRequest(%q, urls, body)", jstr(meth.m,
> "httpMethod"))
> +       // Replace param values after NewRequest to avoid reencoding them.
> +       // E.g. Cloud Storage API requires '%2F' in entity param to be
> kept, but url.Parse replaces it by '/'.
> +       for _, arg := range args.forLocation("path") {
> +               p("\treq.URL.Path = strings.Replace(req.URL.Path,
> \"{%s}\", %s, 1)\n", arg.apiname, arg.expr("c."))
> +       }
> +       // Set opaque to avoid encoding of the parameters in the URL path.
> +       p("\treq.URL.Opaque = req.URL.Path\n")
> +
>         if meth.supportsMedia() {
>                 pn("if hasMedia_ { req.ContentLength = contentLength_ }")
>         }
> @@ -1318,13 +1322,13 @@
>         return a.goname + " " + a.gotype
>  }
>
> -func (a *argument) cleanExpr(prefix string) string {
> +func (a *argument) expr(prefix string) string {
>         switch a.gotype {
>         case "[]string":
>                 log.Printf("TODO(bradfitz): only including the first
> parameter in path query.")
> -               return "cleanPathString(" + prefix + a.goname + "[0])"
> +               return prefix + a.goname + "[0]"
>         case "string":
> -               return "cleanPathString(" + prefix + a.goname + ")"
> +               return prefix + a.goname
>         case "integer", "int64":
>                 return "strconv.FormatInt(" + prefix + a.goname + ", 10)"
>         case "uint64":
>
>
>

[francesc, 2013-05-22 20:31:52]

I'm also testing other APIs (taskqueue, for instance) but I don't know if
there's a way to test all of them.

After discussion with Fred Sauer, he says the best solution is  to URL
encode all values using the equivalent to
urllib.quote_plus<http://docs.python.org/2/library/urllib.html#urllib.quote_plus>

This means that when the value is "foo/bar" it will be encoded as
"foo%2Fbar" which is what I'm actually looking for.

Do you know if we have an equivalent to it in the standard library? I'm
looking for it and I can't find it


On Wed, May 22, 2013 at 1:08 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

> does this break other APIs, though?
>
> I'm afraid you're only testing cloud storage.
>
>
>
> On Wed, May 22, 2013 at 12:58 PM, <campoy@golang.org> wrote:
>
>> Reviewers: bradfitz, adg,
>>
>> Message:
>> Hello bradfitz@golang.org, adg@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<https://code.google.com/p/go...
>>
>>
>> Description:
>> google-api-go-client: avoid reencoding parameters in the URL path
>>
>> Some APIs, such as cloud storage, need to use values that are being
>> now removed.
>> '%2F' is replaced by '/' url.Parse, which is called by http.NewRequest.
>>
>> Please review this at
https://codereview.appspot.**com/9671043/<https://codereview.appspot.com/9671...
>>
>> Affected files:
>>   M google-api-go-generator/gen.go
>>
>>
>> Index: google-api-go-generator/gen.go
>> ==============================**==============================**=======
>> --- a/google-api-go-generator/gen.**go
>> +++ b/google-api-go-generator/gen.**go
>> @@ -488,7 +488,6 @@
>>                 res.generateMethods()
>>         }
>>
>> -       pn("\nfunc cleanPathString(s string) string { return
>> strings.Map(func(r rune) rune { if r >= 0x2d && r <= 0x7a || r == '~' {
>> return r }; return -1 }, s) }")
>>         return nil
>>  }
>>
>> @@ -1137,9 +1136,6 @@
>>                 pn(`params.Set("uploadType", "multipart")`)
>>                 pn("}")
>>         }
>> -       for _, arg := range args.forLocation("path") {
>> -               p("\turls = strings.Replace(urls, \"{%s}\", %s, 1)\n",
>> arg.apiname, arg.cleanExpr("c."))
>> -       }
>>         pn("urls += \"?\" + params.Encode()")
>>         if meth.supportsMedia() {
>>                 if !hasContentType { // Support mediaUpload but no ctype
>> set.
>> @@ -1150,6 +1146,14 @@
>>                 pn("contentLength_, hasMedia_ := googleapi.**
>> ConditionallyIncludeMedia(c.**media_, &body, &ctype)")
>>         }
>>         pn("req, _ := http.NewRequest(%q, urls, body)", jstr(meth.m,
>> "httpMethod"))
>> +       // Replace param values after NewRequest to avoid reencoding them.
>> +       // E.g. Cloud Storage API requires '%2F' in entity param to be
>> kept, but url.Parse replaces it by '/'.
>> +       for _, arg := range args.forLocation("path") {
>> +               p("\treq.URL.Path = strings.Replace(req.URL.Path,
>> \"{%s}\", %s, 1)\n", arg.apiname, arg.expr("c."))
>> +       }
>> +       // Set opaque to avoid encoding of the parameters in the URL path.
>> +       p("\treq.URL.Opaque = req.URL.Path\n")
>> +
>>         if meth.supportsMedia() {
>>                 pn("if hasMedia_ { req.ContentLength = contentLength_ }")
>>         }
>> @@ -1318,13 +1322,13 @@
>>         return a.goname + " " + a.gotype
>>  }
>>
>> -func (a *argument) cleanExpr(prefix string) string {
>> +func (a *argument) expr(prefix string) string {
>>         switch a.gotype {
>>         case "[]string":
>>                 log.Printf("TODO(bradfitz): only including the first
>> parameter in path query.")
>> -               return "cleanPathString(" + prefix + a.goname + "[0])"
>> +               return prefix + a.goname + "[0]"
>>         case "string":
>> -               return "cleanPathString(" + prefix + a.goname + ")"
>> +               return prefix + a.goname
>>         case "integer", "int64":
>>                 return "strconv.FormatInt(" + prefix + a.goname + ", 10)"
>>         case "uint64":
>>
>>
>>
>


-- 
--
Francesc

[francesc, 2013-05-22 23:22:54]

I found url.QueryEscape that does exactly what I wanted.

PTAL

[adg, 2013-05-22 23:24:45]

http://golang.org/pkg/net/url/#QueryEscape ?

[adg, 2013-05-22 23:28:55]

I'd like to see all the changes to generated code in this CL, too. Not sure if
that's what brad has done in the past also.

https://codereview.appspot.com/9671043/diff/12001/google-api-go-generator/gen.go
File google-api-go-generator/gen.go (right):

https://codereview.appspot.com/9671043/diff/12001/google-api-go-generator/gen...
google-api-go-generator/gen.go:1150: // E.g. Cloud Storage API requires '%2F' in
entity param to be kept, but url.Parse replaces it by '/'.
s/by/with/

https://codereview.appspot.com/9671043/diff/12001/google-api-go-generator/gen...
google-api-go-generator/gen.go:1152: p("\treq.URL.Path =
strings.Replace(req.URL.Path, \"{%s}\", %s, 1)\n", arg.apiname,
arg.cleanExpr("c."))
use pn(`req.URL ...`, ...)

https://codereview.appspot.com/9671043/diff/12001/google-api-go-generator/gen...
google-api-go-generator/gen.go:1155: p("\treq.URL.Opaque = req.URL.Path\n")
pn()

[bradfitz, 2013-05-22 23:35:31]

On Wed, May 22, 2013 at 4:28 PM, <adg@golang.org> wrote:

> I'd like to see all the changes to generated code in this CL, too. Not
> sure if that's what brad has done in the past also.


Agree.

[francesc, 2013-05-22 23:39:13]

All the changes are here: https://codereview.appspot.com/9672043/

https://codereview.appspot.com/9671043/diff/12001/google-api-go-generator/gen.go
File google-api-go-generator/gen.go (right):

https://codereview.appspot.com/9671043/diff/12001/google-api-go-generator/gen...
google-api-go-generator/gen.go:1150: // E.g. Cloud Storage API requires '%2F' in
entity param to be kept, but url.Parse replaces it by '/'.
On 2013/05/22 23:28:55, adg wrote:
> s/by/with/

Done.

https://codereview.appspot.com/9671043/diff/12001/google-api-go-generator/gen...
google-api-go-generator/gen.go:1152: p("\treq.URL.Path =
strings.Replace(req.URL.Path, \"{%s}\", %s, 1)\n", arg.apiname,
arg.cleanExpr("c."))
On 2013/05/22 23:28:55, adg wrote:
> use pn(`req.URL ...`, ...)

Done.

https://codereview.appspot.com/9671043/diff/12001/google-api-go-generator/gen...
google-api-go-generator/gen.go:1155: p("\treq.URL.Opaque = req.URL.Path\n")
On 2013/05/22 23:28:55, adg wrote:
> pn()

Done.

[bradfitz, 2013-05-22 23:45:43]

is this really the right use of req.URL.Opaque?

I remember knowing this once, but it feels wrong and doesn't seem to match
the docs at http://golang.org/pkg/net/url/#URL




On Wed, May 22, 2013 at 4:39 PM, <campoy@golang.org> wrote:

> All the changes are here:
https://codereview.appspot.**com/9672043/<https://codereview.appspot.com/9672...
>
>
>
>
>
> https://codereview.appspot.**com/9671043/diff/12001/google-**
>
api-go-generator/gen.go<https://codereview.appspot.com/9671043/diff/12001/google-api-go-generator/gen.go>
> File google-api-go-generator/gen.go (right):
>
> https://codereview.appspot.**com/9671043/diff/12001/google-**
>
api-go-generator/gen.go#**newcode1150<https://codereview.appspot.com/9671043/diff/12001/google-api-go-generator/gen.go#newcode1150>
> google-api-go-generator/gen.**go:1150: // E.g. Cloud Storage API requires
> '%2F' in entity param to be kept, but url.Parse replaces it by '/'.
> On 2013/05/22 23:28:55, adg wrote:
>
>> s/by/with/
>>
>
> Done.
>
>
> https://codereview.appspot.**com/9671043/diff/12001/google-**
>
api-go-generator/gen.go#**newcode1152<https://codereview.appspot.com/9671043/diff/12001/google-api-go-generator/gen.go#newcode1152>
> google-api-go-generator/gen.**go:1152: p("\treq.URL.Path =
> strings.Replace(req.URL.Path, \"{%s}\", %s, 1)\n", arg.apiname,
> arg.cleanExpr("c."))
> On 2013/05/22 23:28:55, adg wrote:
>
>> use pn(`req.URL ...`, ...)
>>
>
> Done.
>
>
> https://codereview.appspot.**com/9671043/diff/12001/google-**
>
api-go-generator/gen.go#**newcode1155<https://codereview.appspot.com/9671043/diff/12001/google-api-go-generator/gen.go#newcode1155>
> google-api-go-generator/gen.**go:1155: p("\treq.URL.Opaque =
> req.URL.Path\n")
> On 2013/05/22 23:28:55, adg wrote:
>
>> pn()
>>
>
> Done.
>
>
https://codereview.appspot.**com/9671043/<https://codereview.appspot.com/9671...
>

[adg, 2013-05-22 23:52:00]

On 23 May 2013 09:45, Brad Fitzpatrick <bradfitz@golang.org> wrote:

> is this really the right use of req.URL.Opaque?


I don't think it is. Used in this way, would drop the host from the request
URL:

http://play.golang.org/p/xn0SO9y2mC

[adg, 2013-05-23 00:27:58]

https://codereview.appspot.com/9671043/diff/17001/google-api-go-generator/gen.go
File google-api-go-generator/gen.go (right):

https://codereview.appspot.com/9671043/diff/17001/google-api-go-generator/gen...
google-api-go-generator/gen.go:1155: pn("req.URL.Opaque = req.URL.Path\n")
I think this should be 

pn(`req.URL.Opaque = "//" + req.URL.Host + req.URL.Path`)

[francesc, 2013-05-23 00:41:35]

I tested and it works with your solution too.

Opaque fields are opaque ...

https://codereview.appspot.com/9671043/diff/17001/google-api-go-generator/gen.go
File google-api-go-generator/gen.go (right):

https://codereview.appspot.com/9671043/diff/17001/google-api-go-generator/gen...
google-api-go-generator/gen.go:1155: pn("req.URL.Opaque = req.URL.Path\n")
On 2013/05/23 00:27:59, adg wrote:
> I think this should be 
> 
> pn(`req.URL.Opaque = "//" + req.URL.Host + req.URL.Path`)

Done.

[adg, 2013-05-23 00:45:36]

I'm a bit worried about your tests if it worked before. The way you had it would
have clobbered the hostname, making the url unusable.

https://codereview.appspot.com/9671043/diff/19002/google-api-go-generator/gen.go
File google-api-go-generator/gen.go (right):

https://codereview.appspot.com/9671043/diff/19002/google-api-go-generator/gen...
google-api-go-generator/gen.go:1152: pn("req.URL.Path =
strings.Replace(req.URL.Path, \"{%s}\", %s, 1)\n", arg.apiname,
arg.cleanExpr("c."))
AGAIN, please use `backtics`  to avoid the needless quoting

https://codereview.appspot.com/9671043/diff/19002/google-api-go-generator/gen...
google-api-go-generator/gen.go:1155: pn("req.URL.Opaque =  \"//\" + req.URL.Host
+ req.URL.Path\n")
ditto

[francesc, 2013-05-23 00:53:23]

https://codereview.appspot.com/9671043/diff/19002/google-api-go-generator/gen.go
File google-api-go-generator/gen.go (right):

https://codereview.appspot.com/9671043/diff/19002/google-api-go-generator/gen...
google-api-go-generator/gen.go:1152: pn("req.URL.Path =
strings.Replace(req.URL.Path, \"{%s}\", %s, 1)\n", arg.apiname,
arg.cleanExpr("c."))
On 2013/05/23 00:45:36, adg wrote:
> AGAIN, please use `backtics`  to avoid the needless quoting

Done
I was keeping it that way just to be consistent with the code around it.

https://codereview.appspot.com/9671043/diff/19002/google-api-go-generator/gen...
google-api-go-generator/gen.go:1155: pn("req.URL.Opaque =  \"//\" + req.URL.Host
+ req.URL.Path\n")
On 2013/05/23 00:45:36, adg wrote:
> ditto

Done.

[adg, 2013-05-23 02:31:38]

This looks ok to me now, but wait for brad.

https://codereview.appspot.com/9671043/diff/19002/google-api-go-generator/gen.go
File google-api-go-generator/gen.go (right):

https://codereview.appspot.com/9671043/diff/19002/google-api-go-generator/gen...
google-api-go-generator/gen.go:1152: pn("req.URL.Path =
strings.Replace(req.URL.Path, \"{%s}\", %s, 1)\n", arg.apiname,
arg.cleanExpr("c."))
On 2013/05/23 00:53:23, gocampoy wrote:
> On 2013/05/23 00:45:36, adg wrote:
> > AGAIN, please use `backtics`  to avoid the needless quoting
> 
> Done
> I was keeping it that way just to be consistent with the code around it.

Look at the surrounding code. It uses backticks everywhere there are quotes
inside the strings.

[francesc, 2013-05-23 17:35:14]

Gentle ping for Brad


On Wed, May 22, 2013 at 7:31 PM, <adg@golang.org> wrote:

> This looks ok to me now, but wait for brad.
>
>
>
> https://codereview.appspot.**com/9671043/diff/19002/google-**
>
api-go-generator/gen.go<https://codereview.appspot.com/9671043/diff/19002/google-api-go-generator/gen.go>
> File google-api-go-generator/gen.go (right):
>
> https://codereview.appspot.**com/9671043/diff/19002/google-**
>
api-go-generator/gen.go#**newcode1152<https://codereview.appspot.com/9671043/diff/19002/google-api-go-generator/gen.go#newcode1152>
> google-api-go-generator/gen.**go:1152: pn("req.URL.Path =
> strings.Replace(req.URL.Path, \"{%s}\", %s, 1)\n", arg.apiname,
> arg.cleanExpr("c."))
> On 2013/05/23 00:53:23, gocampoy wrote:
>
>> On 2013/05/23 00:45:36, adg wrote:
>> > AGAIN, please use `backtics`  to avoid the needless quoting
>>
>
>  Done
>> I was keeping it that way just to be consistent with the code around
>>
> it.
>
> Look at the surrounding code. It uses backticks everywhere there are
> quotes inside the strings.
>
>
https://codereview.appspot.**com/9671043/<https://codereview.appspot.com/9671...
>



-- 
--
Francesc

[bradfitz, 2013-05-23 21:15:47]

LGTM

I think.

Be sure to ask people to test it go golang-nuts after you push out the generated
changes.

I'd also like to see some sample code / tests checked in at some point.

[francesc, 2013-05-23 21:18:29]

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

google-api-go-client: avoid reencoding parameters in the URL path

Some APIs, such as cloud storage, need to use values that are being
now removed.
'%2F' is replaced by '/' url.Parse, which is called by http.NewRequest.

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