CurtHagenlocher commented on code in PR #3177:
URL: https://github.com/apache/arrow-adbc/pull/3177#discussion_r2219877656


##########
csharp/src/Drivers/Databricks/DatabricksConnection.cs:
##########
@@ -334,44 +336,78 @@ protected override HttpMessageHandler CreateHttpHandler()
                 baseHandler = new RetryHttpHandler(baseHandler, 
TemporarilyUnavailableRetryTimeout);
             }
 
-            // Add OAuth handler if OAuth authentication is being used
+            // Add OAuth client credentials handler if OAuth M2M 
authentication is being used
             if (Properties.TryGetValue(SparkParameters.AuthType, out string? 
authType) &&
                 SparkAuthTypeParser.TryParse(authType, out SparkAuthType 
authTypeValue) &&
                 authTypeValue == SparkAuthType.OAuth &&
                 Properties.TryGetValue(DatabricksParameters.OAuthGrantType, 
out string? grantTypeStr) &&
                 DatabricksOAuthGrantTypeParser.TryParse(grantTypeStr, out 
DatabricksOAuthGrantType grantType) &&
                 grantType == DatabricksOAuthGrantType.ClientCredentials)
             {
-                // Note: We assume that properties have already been validated
-                if (Properties.TryGetValue(SparkParameters.HostName, out 
string? host) && !string.IsNullOrEmpty(host))
-                {
-                    // Use hostname directly if provided
-                }
-                else if (Properties.TryGetValue(AdbcOptions.Uri, out string? 
uri) && !string.IsNullOrEmpty(uri))
-                {
-                    // Extract hostname from URI if URI is provided
-                    if (Uri.TryCreate(uri, UriKind.Absolute, out Uri? 
parsedUri))
-                    {
-                        host = parsedUri.Host;
-                    }
-                }
+                string host = GetHost();
 
                 Properties.TryGetValue(DatabricksParameters.OAuthClientId, out 
string? clientId);
                 Properties.TryGetValue(DatabricksParameters.OAuthClientSecret, 
out string? clientSecret);
                 Properties.TryGetValue(DatabricksParameters.OAuthScope, out 
string? scope);
 
-                HttpClient OauthHttpClient = new 
HttpClient(HiveServer2TlsImpl.NewHttpClientHandler(TlsOptions, 
_proxyConfigurator));
+                if (_httpClient == null)
+                {
+                    _httpClient = new 
HttpClient(HiveServer2TlsImpl.NewHttpClientHandler(TlsOptions, 
_proxyConfigurator));
+                }
 
                 var tokenProvider = new OAuthClientCredentialsProvider(
-                    OauthHttpClient,
+                    _httpClient,
                     clientId!,
                     clientSecret!,
                     host!,
                     scope: scope ?? "sql",
                     timeoutMinutes: 1
                 );
 
-                return new OAuthDelegatingHandler(baseHandler, tokenProvider);
+                baseHandler = new OAuthDelegatingHandler(baseHandler, 
tokenProvider);
+            }
+
+            // Add token exchange handler if token renewal is enabled and the 
auth type is OAuth access token

Review Comment:
   This is mutually exclusive with the `if` at line 340, yes? The former is for 
client_credentials while this is for a user token? I'm wondering whether there 
might be some way of making this more obvious in the code, because 
`_httpClient` is being initialized in both but differently and it looked a bit 
tricky to reason about. Perhaps this second `if` could instead be an `else if`?



##########
csharp/src/Drivers/Databricks/DatabricksConnection.cs:
##########
@@ -334,44 +336,78 @@ protected override HttpMessageHandler CreateHttpHandler()
                 baseHandler = new RetryHttpHandler(baseHandler, 
TemporarilyUnavailableRetryTimeout);
             }
 
-            // Add OAuth handler if OAuth authentication is being used
+            // Add OAuth client credentials handler if OAuth M2M 
authentication is being used
             if (Properties.TryGetValue(SparkParameters.AuthType, out string? 
authType) &&
                 SparkAuthTypeParser.TryParse(authType, out SparkAuthType 
authTypeValue) &&
                 authTypeValue == SparkAuthType.OAuth &&
                 Properties.TryGetValue(DatabricksParameters.OAuthGrantType, 
out string? grantTypeStr) &&
                 DatabricksOAuthGrantTypeParser.TryParse(grantTypeStr, out 
DatabricksOAuthGrantType grantType) &&
                 grantType == DatabricksOAuthGrantType.ClientCredentials)
             {
-                // Note: We assume that properties have already been validated
-                if (Properties.TryGetValue(SparkParameters.HostName, out 
string? host) && !string.IsNullOrEmpty(host))
-                {
-                    // Use hostname directly if provided
-                }
-                else if (Properties.TryGetValue(AdbcOptions.Uri, out string? 
uri) && !string.IsNullOrEmpty(uri))
-                {
-                    // Extract hostname from URI if URI is provided
-                    if (Uri.TryCreate(uri, UriKind.Absolute, out Uri? 
parsedUri))
-                    {
-                        host = parsedUri.Host;
-                    }
-                }
+                string host = GetHost();
 
                 Properties.TryGetValue(DatabricksParameters.OAuthClientId, out 
string? clientId);
                 Properties.TryGetValue(DatabricksParameters.OAuthClientSecret, 
out string? clientSecret);
                 Properties.TryGetValue(DatabricksParameters.OAuthScope, out 
string? scope);
 
-                HttpClient OauthHttpClient = new 
HttpClient(HiveServer2TlsImpl.NewHttpClientHandler(TlsOptions, 
_proxyConfigurator));
+                if (_httpClient == null)

Review Comment:
   Can this method be called correctly more than once, and if so, then under 
what circumstances? If it can't be, then there's no need for the null check 
here. It could be replaced e.g. by a `Debug.Assert(_httpClient == null)`. This 
would make the method a little easier to reason about.



##########
csharp/src/Drivers/Databricks/DatabricksConnection.cs:
##########
@@ -334,44 +336,78 @@ protected override HttpMessageHandler CreateHttpHandler()
                 baseHandler = new RetryHttpHandler(baseHandler, 
TemporarilyUnavailableRetryTimeout);
             }
 
-            // Add OAuth handler if OAuth authentication is being used
+            // Add OAuth client credentials handler if OAuth M2M 
authentication is being used
             if (Properties.TryGetValue(SparkParameters.AuthType, out string? 
authType) &&
                 SparkAuthTypeParser.TryParse(authType, out SparkAuthType 
authTypeValue) &&
                 authTypeValue == SparkAuthType.OAuth &&
                 Properties.TryGetValue(DatabricksParameters.OAuthGrantType, 
out string? grantTypeStr) &&
                 DatabricksOAuthGrantTypeParser.TryParse(grantTypeStr, out 
DatabricksOAuthGrantType grantType) &&
                 grantType == DatabricksOAuthGrantType.ClientCredentials)
             {
-                // Note: We assume that properties have already been validated
-                if (Properties.TryGetValue(SparkParameters.HostName, out 
string? host) && !string.IsNullOrEmpty(host))
-                {
-                    // Use hostname directly if provided
-                }
-                else if (Properties.TryGetValue(AdbcOptions.Uri, out string? 
uri) && !string.IsNullOrEmpty(uri))
-                {
-                    // Extract hostname from URI if URI is provided
-                    if (Uri.TryCreate(uri, UriKind.Absolute, out Uri? 
parsedUri))
-                    {
-                        host = parsedUri.Host;
-                    }
-                }
+                string host = GetHost();
 
                 Properties.TryGetValue(DatabricksParameters.OAuthClientId, out 
string? clientId);
                 Properties.TryGetValue(DatabricksParameters.OAuthClientSecret, 
out string? clientSecret);
                 Properties.TryGetValue(DatabricksParameters.OAuthScope, out 
string? scope);
 
-                HttpClient OauthHttpClient = new 
HttpClient(HiveServer2TlsImpl.NewHttpClientHandler(TlsOptions, 
_proxyConfigurator));
+                if (_httpClient == null)
+                {
+                    _httpClient = new 
HttpClient(HiveServer2TlsImpl.NewHttpClientHandler(TlsOptions, 
_proxyConfigurator));

Review Comment:
   I know this was already creating a handler before, but should the 
credential-related handlers be adding the tracing-related functionality and/or 
the retry functionality? Right now, tracing is only added to the primary (data) 
handler and not to either of the credential handlers and retry is being added 
only to the token exchange path. The code is complex enough that it's not clear 
whether this is deliberate or accidental.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Reply via email to