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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
