feat: Introduce API Key support into ClientBuilderBase

[jskeet]

The intention is to make it easy for APIs which do support API keys to expose that, without providing misleading properties to other client builders.

[jskeet]

googleapis/google-cloud-dotnet#11217 shows how this would be used.

[amanda-tarafa]

I'm wondering, why can't we add a default implementation for Build and BuildAsync in here (ClientBuilderBase), instead of generating them. If we do that, then we can call GetEffectiveSettings here in the way we need to.

[jskeet]

It needs to be able to call a concrete constructor in the client type, and use a settings type which is concrete (and which we're non-generic in).
But we may be able to reduce it further (in the next major version of GAX, of course).

[amanda-tarafa]

Ah yes, we'd need a new majot version for that, and not really much we can do otherwise. Looks good.

[jskeet]

A different alternative to consider:

• Most of the API key support in the base class is as it was before
• Remove GetEffectiveSettings, but add a protected AdditionalCallSettings property in the base class which normally returns null, but would return any extra call settings, e.g. for an API key or anything else we want
• Either generate an EffectiveSettings property, or generate an internal static GetEffectiveSettings(XyzSettings settings, CallSettings additionalSettings) method in each concrete settings class.

Generating actual implementation feels nasty, but so does the GetEffectiveSettings method in this PR. Urgh. I'm still thinking about this, basically.

[jskeet]

I've been thinking about this from the perspective of creating clones unnecessarily... and I'm now not that bothered. It's only once per client creation and it's only if the user has specified settings explicitly (which will be pretty rare).

[jskeet]

I've continued thinking about this, and while the whole "clone the settings" part is a bit annoying, I think it's okay for now. Maybe in the next GAX major version we should make the builder generic in the settings type as well, which would allow us to generate less per-API code, I suspect. Will note that, but for now I'm happy to go with this, and change the generator to unconditionally generate the call to GetEffectiveSettings. (We should look into it for hand-written client builders...)

Thoughts?

[amanda-tarafa]

I've been thinking about this as well and I haven't come up with anything that's clean. A tweak to your alternative solution might be to:

• Like yours: Add the CallSettings AdditionalCallSettings property on the base client builder and have the base client builder deal with everything API Key related (and other similar ones in the future).
• On the XYZClient change the generated Create method to accept both XYZSettings and CallSettings additionalCallSettings.
• Have the XYZClient.Create method calculate the effective XYZSettings by cloning and merging the passed XYZSettings with the additionalCallSettings if necessary.

It's still generating implementation but in a place where we are already generating related implementation, for instance XYZClientImpl's constructor calculates effective settings in case the passed settings are null, so, it's just a little bit more of that.

Otherwise, this is fine as well. I do +1 strongly making the client builder base generic on the settings type on the next major version.

[jskeet]

I've been wanting to avoid adding a new Create overload - otherwise I completely agree that would be a good call. No huge urgency for this, so I'll keep pondering options.

[jskeet]

It's been a couple of months now, and I haven't figured out anything better than this. Shall we just go with this? I can't remember offhand how easy it is to write tests for this sort of thing, but I can definitely look into it.

[amanda-tarafa]

Yep, I'm fine with this approach, I don't think we can do much better.

[jskeet]

This is now ready for review and merging. There are tests for GetEffectiveSettings - a lot of the rest is hard to test due to ADC.