feat(gensupport): pass in headers via context

[tritone]

This allows headers added to the context metadata to be applied to the request. See googleapis/gax-go#291.

Needs a release & version bump of gax-go to work.

[noahdietz]

I see what you mean by the nested loop...alternatively, you could do this:

for k, vals := range headers {
  req.Header.Set(k, append(r.Header.Get(k), vals...))
}

[tritone]

Thanks @noahdietz , I think I'll just stick with what I have since it's slightly clearer to look at to me if that's okay.

[gcf-merge-on-green]

Merge-on-green attempted to merge your PR for 6 hours, but it was not mergeable because either one of your required status checks failed, one of your required reviews was not approved, or there is a do not merge label. Learn more about your required status checks here: https://help.github.com/en/github/administering-a-repository/enabling-required-status-checks. You can remove and reapply the label to re-run the bot.

[noahdietz]

Here is the failing test (save you a click)

--- FAIL: TestMedia (0.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x30 pc=0xbf47ff]

goroutine 21 [running]:
testing.tRunner.func1.2({0xcacd80, 0x1302a50})
	/usr/local/go/src/testing/testing.go:1209 +0x36c
testing.tRunner.func1()
	/usr/local/go/src/testing/testing.go:1212 +0x3b6
panic({0xcacd80, 0x1302a50})
	/usr/local/go/src/runtime/panic.go:1047 +0x266
github.com/googleapis/gax-go/v2/callctx.HeadersFromContext(...)
	/root/go/pkg/mod/github.com/googleapis/gax-go/v2@v2.12.0/callctx/callctx.go:50
google.golang.org/api/internal/gensupport.SendRequest({0x0, 0x0}, 0xd75bc2, 0xc000572000)
	/root/go/src/google.golang.org/api/internal/gensupport/send.go:48 +0x5f
google.golang.org/api/storage/v1.(*ObjectsInsertCall).doRequest(0xc000203e38, {0xd745f3, 0x4})
	/root/go/src/google.golang.org/api/storage/v1/storage-gen.go:9901 +0xf2d
google.golang.org/api/storage/v1.(*ObjectsInsertCall).Do(0xc000203e38, {0x0, 0x0, 0x0})
	/root/go/src/google.golang.org/api/storage/v1/storage-gen.go:9913 +0xc5
google.golang.org/api/google-api-go-generator.TestMedia(0xc0002ab380)
	/root/go/src/google.golang.org/api/google-api-go-generator/clients_test.go:75 +0x665
testing.tRunner(0xc0002ab380, 0xdab160)
	/usr/local/go/src/testing/testing.go:1259 +0x230
created by testing.(*T).Run
	/usr/local/go/src/testing/testing.go:1306 +0x727
FAIL	google.golang.org/api/google-api-go-generator	0.073s

[tritone]

Here is the failing test (save you a click)

--- FAIL: TestMedia (0.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x30 pc=0xbf47ff]

goroutine 21 [running]:
testing.tRunner.func1.2({0xcacd80, 0x1302a50})
	/usr/local/go/src/testing/testing.go:1209 +0x36c
testing.tRunner.func1()
	/usr/local/go/src/testing/testing.go:1212 +0x3b6
panic({0xcacd80, 0x1302a50})
	/usr/local/go/src/runtime/panic.go:1047 +0x266
github.com/googleapis/gax-go/v2/callctx.HeadersFromContext(...)
	/root/go/pkg/mod/github.com/googleapis/gax-go/v2@v2.12.0/callctx/callctx.go:50
google.golang.org/api/internal/gensupport.SendRequest({0x0, 0x0}, 0xd75bc2, 0xc000572000)
	/root/go/src/google.golang.org/api/internal/gensupport/send.go:48 +0x5f
google.golang.org/api/storage/v1.(*ObjectsInsertCall).doRequest(0xc000203e38, {0xd745f3, 0x4})
	/root/go/src/google.golang.org/api/storage/v1/storage-gen.go:9901 +0xf2d
google.golang.org/api/storage/v1.(*ObjectsInsertCall).Do(0xc000203e38, {0x0, 0x0, 0x0})
	/root/go/src/google.golang.org/api/storage/v1/storage-gen.go:9913 +0xc5
google.golang.org/api/google-api-go-generator.TestMedia(0xc0002ab380)
	/root/go/src/google.golang.org/api/google-api-go-generator/clients_test.go:75 +0x665
testing.tRunner(0xc0002ab380, 0xdab160)
	/usr/local/go/src/testing/testing.go:1259 +0x230
created by testing.(*T).Run
	/usr/local/go/src/testing/testing.go:1306 +0x727
FAIL	google.golang.org/api/google-api-go-generator	0.073s

Thanks, yeah I started looking into this and interestingly it's possible for the context to be nil here. I think the storage veneer always sets the context on the request but two tests do not. I'm wondering whether the check for a nil context should happen in this library, or in the gax function -- any thoughts?

[noahdietz]

I'm wondering whether the check for a nil context should happen in this library, or in the gax function -- any thoughts?

The context docs say the following:

Do not pass a nil Context, even if a function permits it. Pass context.TODO if you are unsure about which Context to use.

So I don't think we should handle the nil context case. Let's just fix the test?

[tritone]

I'm wondering whether the check for a nil context should happen in this library, or in the gax function -- any thoughts?

The context docs say the following:

Do not pass a nil Context, even if a function permits it. Pass context.TODO if you are unsure about which Context to use.

So I don't think we should handle the nil context case. Let's just fix the test?

So I think it's a bit more complicated in this case because of how the surface is designed. The Apiary methods do not take a context as a required arg but rather a user would have to know to pass it in using a helper method. E.g. the veneer library does this to make an upload call:

		call := c.raw.Objects.Insert(params.bucket, rawObj).
			Media(pr, mediaOpts...).
			Projection("full").
			Context(params.ctx).
			Name(params.attrs.Name)
		// ...
		resp, err = call.Do()

The veneer library of course adds .Context() for all calls but there isn't any docs in Apiary saying that this is mandatory. So, I think I should add a nil check here probably. Does this sound right? (Honestly surprised this hasn't come up before given that it is a bit of a weird assumption.

[noahdietz]

So, I think I should add a nil check here probably. Does this sound right? (Honestly surprised this hasn't come up before given that it is a bit of a weird assumption.

TIL! Yeah this sounds right to me, thanks for explaining.