FAQ
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


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/

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


--

---
You received this message because you are subscribed to the Google Groups "golang-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Search Discussions

  • Brad Fitzpatrick at May 22, 2013 at 8:08 pm
    does this break other APIs, though?

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


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

    --

    ---
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.
  • Francesc Campoy Flores at May 22, 2013 at 8:31 pm
    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 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, 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":


    --
    --
    Francesc

    --

    ---
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedMay 22, '13 at 7:58p
activeMay 22, '13 at 8:31p
posts3
users2
websitegolang.org

People

Translate

site design / logo © 2022 Grokbase