googleapi: use RawPath, not Opaque, for template expansion
Historically the google-api-go-client has had trouble sending certain
characters in paths without the Go standard library (net/url and
net/http packages) double escaping or escaping in an unexpected manner
(for example, space encoding in issue #64).
As a workaround, we started escaping the URL manually and using
url.Opaque (for example, 02cfcab and 5c258d4). This mostly works but has
problems with HTTP/2 (golang/go#16847). In Go 1.5, the URL struct
introduced the RawPath field which was more suitable for this task: if
RawPath is provided and is a valid escaping of Path, then the url
package will use it as the value of EscapedPath (and EscapedPath is then
subsequently used when constructing HTTP requests etc.).
This commit changes uritemplates.Expand to return both the unescaped and
escaped forms of the the template expansion. This allows us to fill in
both url.Path and url.RawPath in a way that satisfies the criteria for
url.EscapedPath to function correctly.
Issue #161.
Change-Id: I51e54e18f198b6465a6d032b1072282bf3d2f9ce
Reviewed-on: https://code-review.googlesource.com/7110
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
diff --git a/google-api-go-generator/storage_test.go b/google-api-go-generator/storage_test.go
index 84a0f9e..15d3209 100644
--- a/google-api-go-generator/storage_test.go
+++ b/google-api-go-generator/storage_test.go
@@ -104,7 +104,7 @@
if g.MultipartForm != nil {
t.Errorf("MultipartForm = %#v; want nil", g.MultipartForm)
}
- if w := s.BasePath + "/b/mybucket/o?alt=json&uploadType=multipart"; g.RequestURI != w {
+ if w := "/b/mybucket/o?alt=json&uploadType=multipart"; g.RequestURI != w {
t.Errorf("RequestURI = %q; want %q", g.RequestURI, w)
}
if w := "\r\n\r\n" + body + "\r\n"; !strings.Contains(string(handler.body), w) {
@@ -247,7 +247,7 @@
if g.MultipartForm != nil {
t.Errorf("MultipartForm = %#v; want nil", g.MultipartForm)
}
- if w := s.BasePath + "/b/mybucket/o?alt=json"; g.RequestURI != w {
+ if w := "/b/mybucket/o?alt=json"; g.RequestURI != w {
t.Errorf("RequestURI = %q; want %q", g.RequestURI, w)
}
if w := `{"bucket":"mybucket","contentEncoding":"utf-8","contentLanguage":"en","contentType":"plain/text","name":"filename"}` + "\n"; string(handler.body) != w {
diff --git a/googleapi/googleapi.go b/googleapi/googleapi.go
index d0cdda6..7157b25 100644
--- a/googleapi/googleapi.go
+++ b/googleapi/googleapi.go
@@ -309,10 +309,10 @@
//
// This calls SetOpaque to avoid encoding of the parameters in the URL path.
func Expand(u *url.URL, expansions map[string]string) {
- expanded, err := uritemplates.Expand(u.Path, expansions)
+ escaped, unescaped, err := uritemplates.Expand(u.Path, expansions)
if err == nil {
- u.Path = expanded
- SetOpaque(u)
+ u.Path = unescaped
+ u.RawPath = escaped
}
}
diff --git a/googleapi/googleapi_test.go b/googleapi/googleapi_test.go
index 4fa051a..498112b 100644
--- a/googleapi/googleapi_test.go
+++ b/googleapi/googleapi_test.go
@@ -180,7 +180,7 @@
map[string]string{
"bucket": "red",
},
- "http://www.golang.org/{bucket/get",
+ "http://www.golang.org/%7Bbucket/get",
},
// "+" prefix for suppressing escape
// See also: http://tools.ietf.org/html/rfc6570#section-3.2.3
@@ -200,7 +200,7 @@
Path: test.in,
}
Expand(&u, test.expansions)
- got := u.Path
+ got := u.EscapedPath()
if got != test.want {
t.Errorf("got %q expected %q in test %d", got, test.want, i+1)
}
diff --git a/googleapi/internal/uritemplates/uritemplates.go b/googleapi/internal/uritemplates/uritemplates.go
index 7c103ba..63bf053 100644
--- a/googleapi/internal/uritemplates/uritemplates.go
+++ b/googleapi/internal/uritemplates/uritemplates.go
@@ -34,11 +34,37 @@
return dst
}
-func escape(s string, allowReserved bool) string {
+// pairWriter is a convenience struct which allows escaped and unescaped
+// versions of the template to be written in parallel.
+type pairWriter struct {
+ escaped, unescaped bytes.Buffer
+}
+
+// Write writes the provided string directly without any escaping.
+func (w *pairWriter) Write(s string) {
+ w.escaped.WriteString(s)
+ w.unescaped.WriteString(s)
+}
+
+// Escape writes the provided string, escaping the string for the
+// escaped output.
+func (w *pairWriter) Escape(s string, allowReserved bool) {
+ w.unescaped.WriteString(s)
if allowReserved {
- return string(reserved.ReplaceAllFunc([]byte(s), pctEncode))
+ w.escaped.Write(reserved.ReplaceAllFunc([]byte(s), pctEncode))
+ } else {
+ w.escaped.Write(unreserved.ReplaceAllFunc([]byte(s), pctEncode))
}
- return string(unreserved.ReplaceAllFunc([]byte(s), pctEncode))
+}
+
+// Escaped returns the escaped string.
+func (w *pairWriter) Escaped() string {
+ return w.escaped.String()
+}
+
+// Unescaped returns the unescaped string.
+func (w *pairWriter) Unescaped() string {
+ return w.unescaped.String()
}
// A uriTemplate is a parsed representation of a URI template.
@@ -170,18 +196,20 @@
return result, err
}
-// Expand expands a URI template with a set of values to produce a string.
-func (t *uriTemplate) Expand(values map[string]string) string {
- var buf bytes.Buffer
+// Expand expands a URI template with a set of values to produce the
+// resultant URI. Two forms of the result are returned: one with all the
+// elements escaped, and one with the elements unescaped.
+func (t *uriTemplate) Expand(values map[string]string) (escaped, unescaped string) {
+ var w pairWriter
for _, p := range t.parts {
- p.expand(&buf, values)
+ p.expand(&w, values)
}
- return buf.String()
+ return w.Escaped(), w.Unescaped()
}
-func (tp *templatePart) expand(buf *bytes.Buffer, values map[string]string) {
+func (tp *templatePart) expand(w *pairWriter, values map[string]string) {
if len(tp.raw) > 0 {
- buf.WriteString(tp.raw)
+ w.Write(tp.raw)
return
}
var first = true
@@ -191,30 +219,30 @@
continue
}
if first {
- buf.WriteString(tp.first)
+ w.Write(tp.first)
first = false
} else {
- buf.WriteString(tp.sep)
+ w.Write(tp.sep)
}
- tp.expandString(buf, term, value)
+ tp.expandString(w, term, value)
}
}
-func (tp *templatePart) expandName(buf *bytes.Buffer, name string, empty bool) {
+func (tp *templatePart) expandName(w *pairWriter, name string, empty bool) {
if tp.named {
- buf.WriteString(name)
+ w.Write(name)
if empty {
- buf.WriteString(tp.ifemp)
+ w.Write(tp.ifemp)
} else {
- buf.WriteString("=")
+ w.Write("=")
}
}
}
-func (tp *templatePart) expandString(buf *bytes.Buffer, t templateTerm, s string) {
+func (tp *templatePart) expandString(w *pairWriter, t templateTerm, s string) {
if len(s) > t.truncate && t.truncate > 0 {
s = s[:t.truncate]
}
- tp.expandName(buf, t.name, len(s) == 0)
- buf.WriteString(escape(s, tp.allowReserved))
+ tp.expandName(w, t.name, len(s) == 0)
+ w.Escape(s, tp.allowReserved)
}
diff --git a/googleapi/internal/uritemplates/uritemplates_test.go b/googleapi/internal/uritemplates/uritemplates_test.go
index bdadea6..a60c4ef 100644
--- a/googleapi/internal/uritemplates/uritemplates_test.go
+++ b/googleapi/internal/uritemplates/uritemplates_test.go
@@ -7,6 +7,7 @@
import (
"fmt"
"log"
+ "net/url"
"testing"
)
@@ -15,7 +16,7 @@
"user": "golang",
"repo": "go",
}
- expanded, err := Expand("https://api.github.com/repos{/user,repo}", values)
+ expanded, _, err := Expand("https://api.github.com/repos{/user,repo}", values)
if err != nil {
log.Fatalf("Error expanding template: %v", err)
}
@@ -50,7 +51,7 @@
}
for _, tt := range testCases {
- exp, err := Expand(tt.tmpl, tt.values)
+ exp, _, err := Expand(tt.tmpl, tt.values)
if err != nil {
t.Errorf("Expand(%q, %v) error: %v", tt.tmpl, tt.values, err)
continue
@@ -208,13 +209,72 @@
{tmpl: "{&var:3}", want: "&var=val"},
}
for _, tt := range testCases {
- exp, err := Expand(tt.tmpl, values)
+ esc, unesc, err := Expand(tt.tmpl, values)
if err != nil {
t.Errorf("Expand(%q) error: %v", tt.tmpl, err)
continue
}
- if exp != tt.want {
- t.Errorf("Expand(%q)\ngot %q\nwant %q", tt.tmpl, exp, tt.want)
+ if esc != tt.want {
+ t.Errorf("Expand(%q)\ngot %q\nwant %q", tt.tmpl, esc, tt.want)
+ }
+ // Check that the escaped form is equivalent to unescaped.
+ urlUnesc, err := url.QueryUnescape(esc)
+ if err != nil {
+ t.Errorf("Expand(%q) gave invalid escaping %q: %v", tt.tmpl, esc, err)
+ continue
+ }
+ if urlUnesc != unesc {
+ t.Errorf("Expand(%q) gave inconsistent escaped/unescaped\nunescaped %q\nescaped %q\nwhich is %q", tt.tmpl, unesc, esc, urlUnesc)
+ }
+ }
+}
+
+func TestExpandUnescaped(t *testing.T) {
+ testCases := []struct {
+ tmpl, wantEsc, wantUnesc string
+ values map[string]string
+ }{
+ {
+ tmpl: "/foo/{bucket}/bar",
+ values: map[string]string{
+ "bucket": "simple",
+ },
+ wantEsc: "/foo/simple/bar",
+ wantUnesc: "/foo/simple/bar",
+ },
+ {
+ tmpl: "/foo/{bucket}/bar",
+ values: map[string]string{
+ "bucket": "path/with/slash",
+ },
+ wantEsc: "/foo/path%2Fwith%2Fslash/bar",
+ wantUnesc: "/foo/path/with/slash/bar",
+ },
+ {
+ tmpl: "/foo/{+bucket}/bar",
+ values: map[string]string{
+ "bucket": "path/with/slash",
+ },
+ wantEsc: "/foo/path/with/slash/bar",
+ wantUnesc: "/foo/path/with/slash/bar",
+ },
+ {
+ tmpl: "/foo/{bucket}/bar",
+ values: map[string]string{
+ "bucket": "double%2Fescaped",
+ },
+ wantEsc: "/foo/double%252Fescaped/bar",
+ wantUnesc: "/foo/double%2Fescaped/bar",
+ },
+ }
+ for _, tt := range testCases {
+ esc, unesc, err := Expand(tt.tmpl, tt.values)
+ if err != nil {
+ t.Errorf("Expand(%q) error: %v", tt.tmpl, err)
+ continue
+ }
+ if esc != tt.wantEsc || unesc != tt.wantUnesc {
+ t.Errorf("Expand(%q)\ngot esc=%q, unesc=%q\nwant esc=%q, unesc=%q", tt.tmpl, esc, unesc, tt.wantEsc, tt.wantUnesc)
}
}
}
diff --git a/googleapi/internal/uritemplates/utils.go b/googleapi/internal/uritemplates/utils.go
index eff260a..2e70b81 100644
--- a/googleapi/internal/uritemplates/utils.go
+++ b/googleapi/internal/uritemplates/utils.go
@@ -4,10 +4,14 @@
package uritemplates
-func Expand(path string, values map[string]string) (string, error) {
+// Expand parses then expands a URI template with a set of values to produce
+// the resultant URI. Two forms of the result are returned: one with all the
+// elements escaped, and one with the elements unescaped.
+func Expand(path string, values map[string]string) (escaped, unescaped string, err error) {
template, err := parse(path)
if err != nil {
- return "", err
+ return "", "", err
}
- return template.Expand(values), nil
+ escaped, unescaped = template.Expand(values)
+ return escaped, unescaped, nil
}
