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