googleapis/google-api-go-client

Path replacement of endpoint in Drive /upload operation

denisazevedo opened this issue · 2 comments

denisazevedo commented

When we create a service with endpoint (option#WithEndpoint) using host and path, ie:
https://myhost.com/connections/google/

	service, err := drive.NewService(ctx, option.WithHTTPClient(client))
	...
	tokenSource := option.WithTokenSource(oauth2.StaticTokenSource(&oauth2.Token{AccessToken: *token}))
	endpointOptions := option.WithEndpoint("https://myhost.com/connections/google/")
	options := []option.ClientOption{
		tokenSource,
		endpointOptions,
	}

it works well for operations like folder creation, as the URL will replaced by:
https://myhost.com/connections/google/files
Replaced by:

google-api-go-client/drive/v3/drive-gen.go

Line 5520 in 0077748

urls := googleapi.ResolveRelative(c.s.BasePath, "files")

But for media upload operations, like file creation, it is removing our endpoint path here:

google-api-go-client/drive/v3/drive-gen.go

Line 5522 in 0077748

urls = googleapi.ResolveRelative(c.s.BasePath, "/upload/drive/v3/files")

Instead of calling this:
https://myhost.com/connections/google/upload/drive/v3/files
it's calling:
https://myhost.com/upload/drive/v3/files

The reason is the slash / at the beggining of the relstr during the googleapi.ResolveRelative func call.
This is the only operation that starts with slash, all others (like files, comments, etc) are appended to our custom endpoint.

Is it an issue or intentional?

noahdietz commented

Interesting, thanks for the detailed report. I kind of think you've been depending on some emergent behavior.

ResolveRelative is documented saying it strips path segments trailing the host:
image.

I reproduced the behavior you've observed here: https://go.dev/play/p/CQFiZZgSsy8

func main() {
	e := "https://foo.googleapis.com/bar/baz/"
	p1 := "v1/fooer"
	p2 := "/v1/fooer"
	p3 := "fooer"

	fmt.Println("resolved endpoint:", googleapi.ResolveRelative(e, ""))
	fmt.Println("resolved p1:", googleapi.ResolveRelative(e, p1))
	fmt.Println("resolved p2:", googleapi.ResolveRelative(e, p2))
	fmt.Println("resolved p3:", googleapi.ResolveRelative(e, p3))
}
resolved endpoint: https://foo.googleapis.com/bar/baz/
resolved p1: https://foo.googleapis.com/bar/baz/v1/fooer
resolved p2: https://foo.googleapis.com/v1/fooer
resolved p3: https://foo.googleapis.com/bar/baz/fooer

You are right that it seems that when the relstr (second argument to ResolveRelative) starts with a /, it strips the path following the host completely. It gets weirder though...

When I take the trailing slash off of the endpoint it starts trimming differently:

resolved endpoint: https://foo.googleapis.com/bar/baz
resolved p1: https://foo.googleapis.com/bar/v1/fooer // different - baz is dropped
resolved p2: https://foo.googleapis.com/v1/fooer // same - both dropped
resolved p3: https://foo.googleapis.com/bar/fooer // different - baz is dropped

This will take a bit of investigating - I am worried about changing ResolveRelative behavior at this point b.c it's used so broadly, and even the "basePaths" in the generated code have path segments following the host name. There are also a few different ways that the endpoint assigned to the client BasePath property could be derived from the environment, which further confuses the potential current usage.

Edit: fix formatting

  • 👍1
denisazevedo commented

Thanks for your attention on this @noahdietz
I see, it really needs a deeper investigation. In our case, it's working broadly because all operations we're using is "appending" the endpoint's path and not replacing it.

About things getting weirder, check this out:
If you run this grep command at root level:

grep --include=\*.go -rn -e 'ResolveRelative(.*, "'

It returns 20k+ occurrences, yes, a lot of them.

But when you run:

grep --include=\*.go -rn -e 'ResolveRelative(.*, "/'

Only 74 starts with a /, and guess what, all of them is /upload/..., for instance:
./checks/v1alpha/checks-gen.go:3096: urls = googleapi.ResolveRelative(c.s.BasePath, "/upload/v1alpha/{+parent}/reports:analyzeUpload")

This commit replaced the old code for the ResolveRelative. I believe it may was a minor mistake when the / was forgotten in the replaced path.

Edit:
Actually, I found this thread where Chris Broadfoot and Seth Hollyman discussed about this.

  • 👍1