feat: Support universe domain for Discovery based libraries by amanda-tarafa · Pull Request #2675 · googleapis/google-api-dotnet-client

amanda-tarafa · 2024-02-15T09:16:31Z

@jskeet as always, one commit at a time. And I've left a note for you to flag the method we are using from generated code.

amanda-tarafa · 2024-02-15T09:17:57Z

+        /// Otherwise <paramref name="defaultUri"/>.
+        /// </list>
+        /// </returns>
+        protected internal string GetEffectiveUri(string explicitUri, string defaultUri)


@jskeet this is the method that we'll be using from generated code to calculate the base URI and the batch URI. I'll send that PR out tomorrow.

jskeet

A few nits, but generally fine.

jskeet · 2024-02-15T11:03:23Z

+public static class HttpRequestMessageExtensions
+{
+    /// <summary>
+    /// Set's the given key/value pair as a request option.


Remove apostrophe.

Done, and on the Get's below as well

jskeet · 2024-02-15T11:04:06Z

+/// <summary>
+/// Extension methods for <see cref="HttpRequestMessage"/>.
+/// </summary>
+public static class HttpRequestMessageExtensions


Rather than making this public, I wonder whether it would be better to have the same code in both projects. That way we can remove it entirely (eventually - it may be a very long time) without affecting the public API at all. We only really want it to be used in our projects, right?

Yep, we only want it in our projects. But it's really in 3 projects that we need this: Google.Apis.Core, Google.Apis and Google.Apis.Auth. And, apart from the triplication, we need to keep it very much in sync because what is set on one side, needs to be equivalently read on the other side.
Plus, the dreprecation path for this is very very clear, with the alternative being "use the .NET provided Options collection instead", which is to say, I think we can deprecate it eventually, and remove after some significant time without a major version.

What do you think?

Actually removing it would then be a binary breaking change, so I'd be nervous about doing that in a minor version.

Given that they need to be kept in sync, how about doing this in one source file that's linked in all three projects? I don't mind it being in the same namespace in all three assemblies.

We discussed offline. A linked file generates some ambiguity issues in test projects so we are going with 3 identical copies.

jskeet · 2024-02-15T11:08:15Z

+            // it for the batch endpoint as well. The batch endpoint does not have an
+            // override mechanism, so we pass explicitUri as null in that case.
+
+            if (explicitUri is not null)


Possibly convert this all into a conditional ?: operator? (Could even use ?? and then a single ?: operator:

explicitUri ?? (UniverseDomain is not null ? defaultUri?.Replace(DefaultUniverseDomain, UniverseDomain) : defaultUri)

jskeet · 2024-02-15T11:09:28Z

+        /// <summary>
+        /// Key for a universe domain in a <see cref="HttpRequestMessage"/> options.
+        /// </summary>
+        public const string UniverseDomainKey = "__UniverseDomainKey";


Does this need to be public?

Not really, but all the other keys here were public already.
And we don't have to duplicate it in Google.Apis.Auth.
Happy to make it internal or private though, let me know.

Would prefer to "default to internal" in general. We've definitely made more things public here than was probably wise (and the Google.Apis vs Google.Apis.Core split doesn't help; something we should consider addressing if we ever create a new major version) but I think it's worth trying to stop adding extra public things that don't need to be public.

Yep, made internal.

jskeet · 2024-02-15T11:33:54Z

+        {
+            HttpRequestMessage request = new HttpRequestMessage();
+
+            GoogleCredential credential = GoogleCredential.FromAccessToken("fake_token");


Maybe just declare this using type IHttpExecuteInterceptor here, to avoid the "as" below?

(Ditto for other tests)

Uy yes, done

jskeet · 2024-02-15T11:35:31Z

+        }
+
+        [Fact]
+        public async Task UniverseDomain_CustomInRequestAndCredential()


Maybe "DifferentCustomInRequestAndCredential"?

jskeet · 2024-02-15T11:35:51Z

+                if (targetUniverseDomain != credentialUniverseDomain)
+                {
+                    throw new InvalidOperationException(
+                    $"The service client universe domain {targetUniverseDomain} does not match the credential universe domain {credentialUniverseDomain}.");


amanda-tarafa

@jskeet I've addressed or replied to all your comments. Changes are in ordered commits starting with "Address review".

Let me know what you think about the two remaining threads regarding visibility. I prefer them as they are, but that's not a hard preference for either.

amanda-tarafa · 2024-02-15T22:25:13Z

+public static class HttpRequestMessageExtensions
+{
+    /// <summary>
+    /// Set's the given key/value pair as a request option.


Done, and on the Get's below as well

amanda-tarafa · 2024-02-15T22:25:45Z

+/// <summary>
+/// Extension methods for <see cref="HttpRequestMessage"/>.
+/// </summary>
+public static class HttpRequestMessageExtensions


Yep, we only want it in our projects. But it's really in 3 projects that we need this: Google.Apis.Core, Google.Apis and Google.Apis.Auth. And, apart from the triplication, we need to keep it very much in sync because what is set on one side, needs to be equivalently read on the other side.
Plus, the dreprecation path for this is very very clear, with the alternative being "use the .NET provided Options collection instead", which is to say, I think we can deprecate it eventually, and remove after some significant time without a major version.

What do you think?

amanda-tarafa · 2024-02-15T23:36:57Z

+            // it for the batch endpoint as well. The batch endpoint does not have an
+            // override mechanism, so we pass explicitUri as null in that case.
+
+            if (explicitUri is not null)


amanda-tarafa · 2024-02-15T23:58:53Z

+        /// <summary>
+        /// Key for a universe domain in a <see cref="HttpRequestMessage"/> options.
+        /// </summary>
+        public const string UniverseDomainKey = "__UniverseDomainKey";


Not really, but all the other keys here were public already.
And we don't have to duplicate it in Google.Apis.Auth.
Happy to make it internal or private though, let me know.

amanda-tarafa · 2024-02-16T00:22:59Z

+        {
+            HttpRequestMessage request = new HttpRequestMessage();
+
+            GoogleCredential credential = GoogleCredential.FromAccessToken("fake_token");


Uy yes, done

amanda-tarafa · 2024-02-16T00:26:35Z

+        }
+
+        [Fact]
+        public async Task UniverseDomain_CustomInRequestAndCredential()


amanda-tarafa · 2024-02-16T00:27:32Z

+                if (targetUniverseDomain != credentialUniverseDomain)
+                {
+                    throw new InvalidOperationException(
+                    $"The service client universe domain {targetUniverseDomain} does not match the credential universe domain {credentialUniverseDomain}.");


jskeet

All "address review" commits look good to me. I have a preference for avoiding the public extension method, but can live with it if you don't want the internal complexity of a linked source file.

amanda-tarafa

@jskeet Outstanding visibility concerns addressed in sorted "Address review" commits. PTAL.

(I'll squash most of these before merging)

amanda-tarafa · 2024-02-21T06:50:37Z

+        /// <summary>
+        /// Key for a universe domain in a <see cref="HttpRequestMessage"/> options.
+        /// </summary>
+        public const string UniverseDomainKey = "__UniverseDomainKey";


Yep, made internal.

amanda-tarafa · 2024-02-21T06:52:23Z

+/// <summary>
+/// Extension methods for <see cref="HttpRequestMessage"/>.
+/// </summary>
+public static class HttpRequestMessageExtensions


We discussed offline. A linked file generates some ambiguity issues in test projects so we are going with 3 identical copies.

jskeet · 2024-02-21T18:06:44Z

+
+using System.Net.Http;
+
+namespace Google.Apis.Util.Extensions;


Why the ".Extensions" bit? We don't have that for TaskExtensions for example.
Perhaps this was left over from trying to do the link version?

It's intentional, because in tests we already import Google.Apis.Util for other things. So, if we don't put these in specific namespaces, we'd still have the problem of ambiguity.

Note that most the namespaces in Google.Apis.Core are missing the Core bit (except for a few newly added and interal classes) so in Google.Apis.Core we also have Google.Apis.Util, so there I had to do Google.Apis.Core.Util.Extensions.

Righto. Will take a closer look in the morning, but approve now as I'm sure it's fine - feel free to merge.

OK, I know what we can do, we can test the one in Google.Apis instead of the one in Google.Apis.Core and then we can get rid of the Extensions in both. We still need to keep the Google.Apis.Core.Util though (instead of Google.Apis.Util that we normally have in Google.Apis.Core).

Another option is to not use them as extension methods in tests - call them with fully-qualified names (Google.Apis.Utils.HttpRequestMessageExtensions.Xyz()) so the collision doesn't matter.

Yes, but that still needs us to have the odd Google.Apis.Core.Utils: it's just Google.Apis.Utils even in Google.Apis.Core, that's the main problem, that in Google.Apis.Core we don't use Core in namespaces. I'm pushing shortly, I've had to use Core or else there's no way to disambiguate, but I was able to drop the Extensions bit.

Pushed:

The third commit is new, so that everything is normalized.

And the two Addres Review commits have changed. We still have to use Google.Apis.Core.Utils in Google.Apis.Core even though the norma would have been to use Google.Apis.Utils.

We don't use Core in Google.Apis.Core, just Google.Apis.

The decision is hidden in HttpRequestMessage extension methods. Note that we had to use the namespace Google.Apis.Core.Utils in the Google.Apis.Core project even though normally it would have been just Google.Apis.Utils.

- The universe domain may be specified through the client service initializer. - A helper method to calculate effective URIs that takes into account the universe domain. This method is to be used by generated code, see googleapis/gapic-generator-csharp#746. - The universe domain is passed as a request option for the credential to validate against when intercepting and before setting the token.

amanda-tarafa · 2024-02-22T17:00:49Z

I've squashed everything into more meaningful commits, I'll merge on green.

Features - Universe domain support: - feat: [#2675](#2675) Universe domain support for Discovery based libraries. - feat: [#2635](#2635) Universe domain support for authentication.

amanda-tarafa requested review from a team and jskeet February 15, 2024 09:16

amanda-tarafa commented Feb 15, 2024

View reviewed changes

jskeet approved these changes Feb 15, 2024

View reviewed changes

amanda-tarafa force-pushed the universe-domain-discovery branch from 60c67de to 5f8263e Compare February 16, 2024 00:49

amanda-tarafa commented Feb 16, 2024

View reviewed changes

jskeet approved these changes Feb 16, 2024

View reviewed changes

amanda-tarafa force-pushed the universe-domain-discovery branch from 5f8263e to ab34224 Compare February 21, 2024 17:56

amanda-tarafa commented Feb 21, 2024

View reviewed changes

jskeet reviewed Feb 21, 2024

View reviewed changes

jskeet approved these changes Feb 21, 2024

View reviewed changes

amanda-tarafa force-pushed the universe-domain-discovery branch from ab34224 to 17f1315 Compare February 21, 2024 18:51

This was referenced Feb 21, 2024

feat: Universe domain support for discovery based libraries. googleapis/gapic-generator-csharp#746

Merged

feat: Universe domain support for Discovery based libraries. googleapis/gax-dotnet#761

Merged

amanda-tarafa added 5 commits February 22, 2024 08:54

chore: Clean usings and similar.

8b6d8c3

chore: Remove unused mock type.

c23ac4b

chore: Normalize Google.Apis.Core namespaces.

bc6447f

We don't use Core in Google.Apis.Core, just Google.Apis.

refactor: Use HttpRequestMessage.Options when possible.

7e72cd7

The decision is hidden in HttpRequestMessage extension methods. Note that we had to use the namespace Google.Apis.Core.Utils in the Google.Apis.Core project even though normally it would have been just Google.Apis.Utils.

amanda-tarafa force-pushed the universe-domain-discovery branch from 17f1315 to 26e3450 Compare February 22, 2024 17:00

amanda-tarafa merged commit ec8fdef into main Feb 22, 2024

amanda-tarafa deleted the universe-domain-discovery branch February 22, 2024 17:02

amanda-tarafa mentioned this pull request Feb 22, 2024

Update support version 1.67.0-beta01 -> 1.67.0 #2683

Merged


		using System.Net.Http;

		namespace Google.Apis.Util.Extensions;

Conversation

amanda-tarafa commented Feb 15, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jskeet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amanda-tarafa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jskeet left a comment

Choose a reason for hiding this comment

Uh oh!

amanda-tarafa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment