google-api-go-generator: generalize media path replacement
This change improves media path handling when dealing with API
methods that support multipart media upload.
By convention, most APIs at Google leverage an /upload/ alternate
prefix path to do with multipart requests, as they rely on a common
infrastructure component that uses this convention. This removes
special path replacement behavior by simply leveraging the
supplied media path as expressed in the discovery doc reference:
https://developers.google.com/discovery/v1/reference/apis
This has the effect of allowing APIs to generate correctly
when using hostnames other than www.googleapis.com as the root
URL, which previous behavior relied upon.
This PR also updates tests, as there were a lot of implicit
behaviors that don't reflect proper usage.
For example, while under test the BasePath was frequently set
to the bare server.URL e.g. http://n.n.n.n:p/ which meant that
things like the prior redirect hack was ignored entirely.
This change also adds a test for googleapi.ResolveRelative to
ensure this path replacement behavior is preserved for future
maintainers.
Change-Id: Icbc9bccf9f168f6c78d17b7feab6d82c6a4a1199
Reviewed-on: https://code-review.googlesource.com/c/google-api-go-client/+/46011
Reviewed-by: Chris Broadfoot <cbro@google.com>
Reviewed-by: kokoro <noreply+kokoro@google.com>
diff --git a/google-api-go-generator/clients_test.go b/google-api-go-generator/clients_test.go
index e7c6165..ade083c 100644
--- a/google-api-go-generator/clients_test.go
+++ b/google-api-go-generator/clients_test.go
@@ -61,7 +61,7 @@
if err != nil {
t.Fatalf("unable to create service: %v", err)
}
- s.BasePath = server.URL
+ s.BasePath = fmt.Sprintf("%s%s", server.URL, "/storage/v1/")
const body = "fake media data"
f := strings.NewReader(body)
@@ -104,7 +104,7 @@
if w := "chunked"; len(g.TransferEncoding) != 1 || g.TransferEncoding[0] != w {
t.Errorf("TransferEncoding = %#v; want %q", g.TransferEncoding, w)
}
- if w := strings.TrimPrefix(s.BasePath, "http://"); g.Host != w {
+ if w := server.Listener.Addr().String(); g.Host != w {
t.Errorf("Host = %q; want %q", g.Host, w)
}
if g.Form != nil {
@@ -116,7 +116,7 @@
if g.MultipartForm != nil {
t.Errorf("MultipartForm = %#v; want nil", g.MultipartForm)
}
- if w := "/b/mybucket/o?alt=json&prettyPrint=false&uploadType=multipart"; g.RequestURI != w {
+ if w := "/upload/storage/v1/b/mybucket/o?alt=json&prettyPrint=false&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) {
@@ -206,7 +206,7 @@
if err != nil {
t.Fatalf("unable to create service: %v", err)
}
- s.BasePath = server.URL
+ s.BasePath = fmt.Sprintf("%s%s", server.URL, "/storage/v1/")
o := &storage.Object{
Bucket: "mybucket",
@@ -247,7 +247,7 @@
if len(g.TransferEncoding) != 0 {
t.Errorf("TransferEncoding = %#v; want []string{}", g.TransferEncoding)
}
- if w := strings.TrimPrefix(s.BasePath, "http://"); g.Host != w {
+ if w := server.Listener.Addr().String(); g.Host != w {
t.Errorf("Host = %q; want %q", g.Host, w)
}
if g.Form != nil {
@@ -259,7 +259,7 @@
if g.MultipartForm != nil {
t.Errorf("MultipartForm = %#v; want nil", g.MultipartForm)
}
- if w := "/b/mybucket/o?alt=json&prettyPrint=false"; g.RequestURI != w {
+ if w := "/storage/v1/b/mybucket/o?alt=json&prettyPrint=false"; 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 {
@@ -280,7 +280,7 @@
if err != nil {
t.Fatalf("unable to create service: %v", err)
}
- s.BasePath = server.URL
+ s.BasePath = fmt.Sprintf("%s%s", server.URL, "/storage/v1/")
const body = "fake media data"
f := strings.NewReader(body)
@@ -367,7 +367,7 @@
t.Fatalf("len(reqURIs) = %v, want %v", g, w)
}
want := []string{
- "/b/mybucket/o?alt=json&ifGenerationMatch=42&name=filename&prettyPrint=false&projection=full&uploadType=resumable",
+ "/upload/storage/v1/b/mybucket/o?alt=json&ifGenerationMatch=42&name=filename&prettyPrint=false&projection=full&uploadType=resumable",
"/uploadURL",
}
if !reflect.DeepEqual(handler.reqURIs, want) {
diff --git a/google-api-go-generator/gen.go b/google-api-go-generator/gen.go
index 9014d60..303168a 100644
--- a/google-api-go-generator/gen.go
+++ b/google-api-go-generator/gen.go
@@ -1949,10 +1949,7 @@
pn("urls := googleapi.ResolveRelative(c.s.BasePath, %q)", meth.m.Path)
if meth.supportsMediaUpload() {
pn("if c.mediaInfo_ != nil {")
- // Hack guess, since we get a 404 otherwise:
- //pn("urls = googleapi.ResolveRelative(%q, %q)", a.apiBaseURL(), meth.mediaUploadPath())
- // Further hack. Discovery doc is wrong?
- pn(" urls = strings.Replace(urls, %q, %q, 1)", "https://www.googleapis.com/", "https://www.googleapis.com/upload/")
+ pn(" urls = googleapi.ResolveRelative(c.s.BasePath, %q)", meth.mediaUploadPath())
pn(` c.urlParams_.Set("uploadType", c.mediaInfo_.UploadType())`)
pn("}")
diff --git a/googleapi/googleapi_test.go b/googleapi/googleapi_test.go
index 059b2fa..76dc483 100644
--- a/googleapi/googleapi_test.go
+++ b/googleapi/googleapi_test.go
@@ -165,6 +165,16 @@
relstr: ":8080foo",
wantPanic: true,
},
+ {
+ basestr: "https://www.googleapis.com/exampleapi/v2/somemethod",
+ relstr: "/upload/exampleapi/v2/somemethod",
+ want: "https://www.googleapis.com/upload/exampleapi/v2/somemethod",
+ },
+ {
+ basestr: "https://otherhost.googleapis.com/exampleapi/v2/somemethod",
+ relstr: "/upload/exampleapi/v2/alternatemethod",
+ want: "https://otherhost.googleapis.com/upload/exampleapi/v2/alternatemethod",
+ },
}
for _, test := range resolveRelativeTests {
diff --git a/internal/kokoro/test.sh b/internal/kokoro/test.sh
index 4ddcc76..c49698c 100755
--- a/internal/kokoro/test.sh
+++ b/internal/kokoro/test.sh
@@ -34,5 +34,8 @@
try3 go mod download
./internal/kokoro/vet.sh
+# Testing the generator itself depends on a generation step
+cd google-api-go-generator; go generate; cd ..
+
# Run tests and tee output to log file, to be pushed to GCS as artifact.
go test -race -v -short ./... 2>&1 | tee $KOKORO_ARTIFACTS_DIR/$KOKORO_GERRIT_CHANGE_NUMBER.txt
