[golang-dev] code review 9671043: google-api-go-client: avoid reencoding parameters in th... (issue 9671043)

On Wed, May 22, 2013 at 12:58 PM, 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/google-api-go-client>


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/9671043/>

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":

On Wed, May 22, 2013 at 1:08 PM, Brad Fitzpatrick wrote:

does this break other APIs, though?

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