Re: [PR] feat(csharp/src/Drivers/BigQuery): Add support for AAD/Entra authentication [arrow-adbc]
CurtHagenlocher merged PR #2655: URL: https://github.com/apache/arrow-adbc/pull/2655 -- 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
Re: [PR] feat(csharp/src/Drivers/BigQuery): Add support for AAD/Entra authentication [arrow-adbc]
CurtHagenlocher commented on code in PR #2655:
URL: https://github.com/apache/arrow-adbc/pull/2655#discussion_r2056429112
##
csharp/src/Drivers/BigQuery/RetryManager.cs:
##
@@ -0,0 +1,90 @@
+
+/*
+* Licensed to the Apache Software Foundation (ASF) under one or more
+* contributor license agreements. See the NOTICE file distributed with
+* this work for additional information regarding copyright ownership.
+* The ASF licenses this file to You under the Apache License, Version 2.0
+* (the "License"); you may not use this file except in compliance with
+* the License. You may obtain a copy of the License at
+*
+*http://www.apache.org/licenses/LICENSE-2.0
+*
+* Unless required by applicable law or agreed to in writing, software
+* distributed under the License is distributed on an "AS IS" BASIS,
+* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+* See the License for the specific language governing permissions and
+* limitations under the License.
+*/
+
+using System;
+using System.Threading.Tasks;
+
+namespace Apache.Arrow.Adbc.Drivers.BigQuery
+{
+///
+/// Class that will retry calling a method with an exponential backoff.
+///
+class RetryManager
+{
+public static async Task ExecuteWithRetriesAsync(
+ Func> action,
+ int maxRetries = 5,
+ int initialDelayMilliseconds = 200)
+{
+return await ExecuteWithRetriesAsync(null, action, maxRetries,
initialDelayMilliseconds);
+}
+
+public static async Task ExecuteWithRetriesAsync(
+ITokenProtectedResource? tokenProtectedResource,
+Func> action,
+int maxRetries = 5,
+int initialDelayMilliseconds = 200)
+{
+if (action == null)
+{
+throw new AdbcException("There is no method to retry",
AdbcStatusCode.InvalidArgument);
+}
+
+int retryCount = 0;
+int delay = initialDelayMilliseconds;
+
+while (retryCount < maxRetries)
+{
+try
+{
+T result = await action();
+return result;
+}
+catch (Exception ex)
+{
+retryCount++;
+if (retryCount >= maxRetries)
+{
+if ((tokenProtectedResource?.UpdateToken != null))
+{
+if
(tokenProtectedResource?.TokenRequiresUpdate(ex) == true)
+{
+throw new AdbcException($"Cannot update access
token after {maxRetries} tries", AdbcStatusCode.Unauthenticated, ex);
+}
+}
+
+throw new AdbcException($"Cannot execute
{action.Method.Name} after {maxRetries} tries", AdbcStatusCode.UnknownError,
ex);
+}
+
+if ((tokenProtectedResource?.UpdateToken != null))
+{
+if (tokenProtectedResource.TokenRequiresUpdate(ex) ==
true)
+{
+await tokenProtectedResource.UpdateToken();
+}
+}
+
+await Task.Delay(delay);
+delay *= 2;
Review Comment:
I'm not sure whether the new code `delay += delay` is supposed to be
different than `delay *= 2`. Did you mean `delay += initialDelayMilliseconds`?
Can I suggest an alternative of something like `delay = Math.Min(2 * delay,
5000)`?
--
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
Re: [PR] feat(csharp/src/Drivers/BigQuery): Add support for AAD/Entra authentication [arrow-adbc]
davidhcoe commented on code in PR #2655:
URL: https://github.com/apache/arrow-adbc/pull/2655#discussion_r2055084479
##
csharp/src/Drivers/BigQuery/BigQueryConnection.cs:
##
@@ -337,33 +419,49 @@ private IArrowArray[] GetCatalogs(
StringArray.Builder catalogNameBuilder = new StringArray.Builder();
List catalogDbSchemasValues = new
List();
string catalogRegexp = PatternToRegEx(catalogPattern);
-PagedEnumerable? catalogs =
this.client?.ListProjects();
+PagedEnumerable? catalogs;
+List projectIds = new List();
-if (catalogs != null)
+try
{
-List projectIds = catalogs.Select(x =>
x.ProjectId).ToList();
+Func?>> func =
() => Task.Run(() =>
+{
+// stick with this call because PagedAsyncEnumerable has
different behaviors for selecting items
+return Client?.ListProjects();
+});
-if (this.includePublicProjectIds &&
!projectIds.Contains(publicProjectId))
-projectIds.Add(publicProjectId);
+catalogs =
ExecuteWithRetriesAsync?>(func).GetAwaiter().GetResult();
+
+if (catalogs != null)
+{
+projectIds = catalogs.Select(x => x.ProjectId).ToList();
+}
+}
+catch
+{
+// TODO: Logging
+}
Review Comment:
removed
--
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
Re: [PR] feat(csharp/src/Drivers/BigQuery): Add support for AAD/Entra authentication [arrow-adbc]
CurtHagenlocher commented on code in PR #2655:
URL: https://github.com/apache/arrow-adbc/pull/2655#discussion_r2054534280
##
csharp/src/Drivers/BigQuery/BigQueryConnection.cs:
##
@@ -58,14 +57,44 @@ public class BigQueryConnection : AdbcConnection
public BigQueryConnection(IReadOnlyDictionary
properties)
{
-this.properties = properties;
+if (properties == null)
+{
+this.properties = new Dictionary();
+}
+else
+{
+this.properties = properties.ToDictionary(k => k.Key, v =>
v.Value);
+}
// add the default value for now and set to true until C# has a
BigDecimal
-Dictionary modifiedProperties =
this.properties.ToDictionary(k => k.Key, v => v.Value);
-modifiedProperties[BigQueryParameters.LargeDecimalsAsString] =
BigQueryConstants.TreatLargeDecimalAsString;
-this.properties = new ReadOnlyDictionary(modifiedProperties);
+this.properties[BigQueryParameters.LargeDecimalsAsString] =
BigQueryConstants.TreatLargeDecimalAsString;
+this.httpClient = new HttpClient();
+
+if
(this.properties.TryGetValue(BigQueryParameters.MaximumRetryAttempts, out
string? sRetryAttempts) &&
+int.TryParse(sRetryAttempts, out int retries) &&
+retries >= 0)
+{
+MaxRetryAttempts = retries;
+}
+
+if (this.properties.TryGetValue(BigQueryParameters.RetryDelayMs,
out string? sRetryDelay) &&
+int.TryParse(sRetryDelay, out int delay) &&
+delay >= 0)
+{
+RetryDelayMs = delay;
+}
}
+public Func? UpdateToken { get; set; }
Review Comment:
Should this get a doc comment?
##
csharp/src/Drivers/BigQuery/BigQueryConnection.cs:
##
@@ -337,33 +419,49 @@ private IArrowArray[] GetCatalogs(
StringArray.Builder catalogNameBuilder = new StringArray.Builder();
List catalogDbSchemasValues = new
List();
string catalogRegexp = PatternToRegEx(catalogPattern);
-PagedEnumerable? catalogs =
this.client?.ListProjects();
+PagedEnumerable? catalogs;
+List projectIds = new List();
-if (catalogs != null)
+try
{
-List projectIds = catalogs.Select(x =>
x.ProjectId).ToList();
+Func?>> func =
() => Task.Run(() =>
+{
+// stick with this call because PagedAsyncEnumerable has
different behaviors for selecting items
+return Client?.ListProjects();
+});
-if (this.includePublicProjectIds &&
!projectIds.Contains(publicProjectId))
-projectIds.Add(publicProjectId);
+catalogs =
ExecuteWithRetriesAsync?>(func).GetAwaiter().GetResult();
+
+if (catalogs != null)
+{
+projectIds = catalogs.Select(x => x.ProjectId).ToList();
+}
+}
+catch
+{
+// TODO: Logging
+}
Review Comment:
We weren't swallowing exceptions here before; why are we doing it now?
Consider adding a comment with the justification.
##
csharp/src/Drivers/BigQuery/BigQueryConnection.cs:
##
@@ -434,7 +538,7 @@ private StructArray GetDbSchemas(
nullBitmapBuffer.Build());
}
-private StructArray GetTableSchemas(
+StructArray GetTableSchemas(
Review Comment:
We should have a documented "preferred coding conventions". Much of the
other C# code in this overall repo is explicitly using "private", and getting
rid of it here has added a bunch of unrelated churn to this PR.
##
csharp/src/Drivers/BigQuery/BigQueryConnection.cs:
##
@@ -36,17 +37,15 @@ namespace Apache.Arrow.Adbc.Drivers.BigQuery
///
/// BigQuery-specific implementation of
///
-public class BigQueryConnection : AdbcConnection
+public class BigQueryConnection : AdbcConnection, ITokenProtectedResource
Review Comment:
I can't say I'm super happy about this, but it's not like it wasn't already
`public` so we can defer a change until perhaps there's better platform support
for the scenario.
##
csharp/src/Drivers/BigQuery/BigQueryStatement.cs:
##
@@ -127,51 +198,57 @@ public override QueryResult ExecuteQuery()
}
}
}
+
ReadSession rs = new ReadSession { Table = table, DataFormat =
DataFormat.Arrow };
-ReadSession rrs = readClient.CreateReadSession("projects/" +
results.TableReference.ProjectId, rs, maxStreamCount);
+ReadSession rrs =
clientMgr.ReadClient.CreateReadSession("projects/" +
results.TableReference.Proj
Re: [PR] feat(csharp/src/Drivers/BigQuery): Add support for AAD/Entra authentication [arrow-adbc]
CurtHagenlocher commented on PR #2655: URL: https://github.com/apache/arrow-adbc/pull/2655#issuecomment-2822032366 > My biggest concern around this change is a lack of clarity about concurrency issues. I had been specifically concerned about a token expiring while we were downloading chunks in parallel, but it turns out the connector is not doing that. -- 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
Re: [PR] feat(csharp/src/Drivers/BigQuery): Add support for AAD/Entra authentication [arrow-adbc]
davidhcoe commented on code in PR #2655:
URL: https://github.com/apache/arrow-adbc/pull/2655#discussion_r2052905139
##
csharp/src/Drivers/BigQuery/BigQueryStatement.cs:
##
@@ -36,88 +37,155 @@ namespace Apache.Arrow.Adbc.Drivers.BigQuery
///
/// BigQuery-specific implementation of
///
-public class BigQueryStatement : AdbcStatement
+class BigQueryStatement : AdbcStatement, ITokenProtectedResource,
IDisposable
{
-readonly BigQueryClient client;
-readonly GoogleCredential credential;
+readonly BigQueryConnection bigQueryConnection;
-public BigQueryStatement(BigQueryClient client, GoogleCredential
credential)
+public BigQueryStatement(BigQueryConnection bigQueryConnection)
{
-this.client = client;
-this.credential = credential;
+if (bigQueryConnection == null) { throw new
AdbcException($"{nameof(bigQueryConnection)} cannot be null",
AdbcStatusCode.InvalidArgument); }
+
+// pass on the handler since this isn't accessible publicly
+UpdateToken = bigQueryConnection.UpdateToken;
+
+this.bigQueryConnection = bigQueryConnection;
}
+public Func? UpdateToken { get; set; }
+
public IReadOnlyDictionary? Options { get; set; }
Review Comment:
The highlighted confused me for a bit but I believe this is related to
Options. Updated in the latest push.
--
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
Re: [PR] feat(csharp/src/Drivers/BigQuery): Add support for AAD/Entra authentication [arrow-adbc]
davidhcoe commented on code in PR #2655: URL: https://github.com/apache/arrow-adbc/pull/2655#discussion_r2052904194 ## csharp/src/Drivers/BigQuery/BigQueryConnection.cs: ## @@ -36,17 +37,15 @@ namespace Apache.Arrow.Adbc.Drivers.BigQuery /// /// BigQuery-specific implementation of /// -public class BigQueryConnection : AdbcConnection +public class BigQueryConnection : AdbcConnection, ITokenProtectedResource Review Comment: If ITokenProtectedResource is public, then BigQueryConnection could be internal. However, if ITokenProtectedResource is internal (which it probably should be) then I think BigQueryConnection should stay public because that would be the only way for a caller to access the UpdateToken functionality. -- 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
Re: [PR] feat(csharp/src/Drivers/BigQuery): Add support for AAD/Entra authentication [arrow-adbc]
davidhcoe commented on code in PR #2655:
URL: https://github.com/apache/arrow-adbc/pull/2655#discussion_r2052900302
##
csharp/src/Drivers/BigQuery/BigQueryUtils.cs:
##
@@ -0,0 +1,45 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+using System;
+using Google;
+
+namespace Apache.Arrow.Adbc.Drivers.BigQuery
+{
+internal class BigQueryUtils
+{
+public static bool TokenRequiresUpdate(Exception ex)
+{
+bool result = false;
+
+switch (ex)
+{
+case GoogleApiException apiException:
+if (apiException.HttpStatusCode ==
System.Net.HttpStatusCode.Unauthorized)
+{
+result = true;
Review Comment:
Changed in latest push.
--
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
Re: [PR] feat(csharp/src/Drivers/BigQuery): Add support for AAD/Entra authentication [arrow-adbc]
davidhcoe commented on code in PR #2655:
URL: https://github.com/apache/arrow-adbc/pull/2655#discussion_r2052899323
##
csharp/src/Drivers/BigQuery/RetryManager.cs:
##
@@ -0,0 +1,90 @@
+
+/*
+* Licensed to the Apache Software Foundation (ASF) under one or more
+* contributor license agreements. See the NOTICE file distributed with
+* this work for additional information regarding copyright ownership.
+* The ASF licenses this file to You under the Apache License, Version 2.0
+* (the "License"); you may not use this file except in compliance with
+* the License. You may obtain a copy of the License at
+*
+*http://www.apache.org/licenses/LICENSE-2.0
+*
+* Unless required by applicable law or agreed to in writing, software
+* distributed under the License is distributed on an "AS IS" BASIS,
+* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+* See the License for the specific language governing permissions and
+* limitations under the License.
+*/
+
+using System;
+using System.Threading.Tasks;
+
+namespace Apache.Arrow.Adbc.Drivers.BigQuery
+{
+///
+/// Class that will retry calling a method with an exponential backoff.
+///
+public class RetryManager
Review Comment:
Only in the sense that it started its journey in the Adbc project but then
was demoted to just BigQuery. Made internal for now.
--
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
Re: [PR] feat(csharp/src/Drivers/BigQuery): Add support for AAD/Entra authentication [arrow-adbc]
davidhcoe commented on code in PR #2655:
URL: https://github.com/apache/arrow-adbc/pull/2655#discussion_r2052898693
##
csharp/src/Drivers/BigQuery/ITokenProtectedResource.cs:
##
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+using System;
+using System.Threading.Tasks;
+
+namespace Apache.Arrow.Adbc.Drivers.BigQuery
+{
+///
+/// Common interface for a token protected resource.
+///
+public interface ITokenProtectedResource
+{
+///
+/// The function to call when updating the token.
+///
+public Func? UpdateToken { get; set; }
+
+///
+/// Determines the token needs to be updated.
+///
+/// The exception that occurs.
+/// True/False indicating a refresh is needed.
+public bool TokenRequiresUpdate(Exception ex);
Review Comment:
I moved this to be an internal only API, but I don't think it can be split
into two different APIs just because RetryManager looks at the
ITokenProtectedResource to determine if the token needs to be updated then
invokes the delegate for the update to occur.
--
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
Re: [PR] feat(csharp/src/Drivers/BigQuery): Add support for AAD/Entra authentication [arrow-adbc]
davidhcoe commented on code in PR #2655:
URL: https://github.com/apache/arrow-adbc/pull/2655#discussion_r2052897371
##
csharp/src/Drivers/BigQuery/BigQueryConnection.cs:
##
@@ -36,17 +37,15 @@ namespace Apache.Arrow.Adbc.Drivers.BigQuery
///
/// BigQuery-specific implementation of
///
-public class BigQueryConnection : AdbcConnection
+public class BigQueryConnection : AdbcConnection, ITokenProtectedResource
{
-readonly IReadOnlyDictionary properties;
-BigQueryClient? client;
-GoogleCredential? credential;
+IReadOnlyDictionary properties;
Review Comment:
Changed in the latest push.
--
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
Re: [PR] feat(csharp/src/Drivers/BigQuery): Add support for AAD/Entra authentication [arrow-adbc]
davidhcoe commented on code in PR #2655:
URL: https://github.com/apache/arrow-adbc/pull/2655#discussion_r2052897073
##
csharp/src/Drivers/BigQuery/BigQueryParameters.cs:
##
@@ -50,10 +54,18 @@ public class BigQueryParameters
public class BigQueryConstants
{
public const string UserAuthenticationType = "user";
+public const string EntraIdAuthenticationType = "aad";
public const string ServiceAccountAuthenticationType = "service";
public const string TokenEndpoint =
"https://accounts.google.com/o/oauth2/token";;
public const string TreatLargeDecimalAsString = "true";
+// Entra ID / Azure AD constants
+public const string GrantType =
"urn:ietf:params:oauth:grant-type:token-exchange";
+public const string SubjectTokenType =
"urn:ietf:params:oauth:token-type:id_token";
+public const string RequestedTokenType =
"urn:ietf:params:oauth:token-type:access_token";
+public const string EntraIdScope =
"https://www.googleapis.com/auth/cloud-platform";;
+public const string StsTokenEndpoint =
"https://sts.googleapis.com/v1/token";;
Review Comment:
Converted to internal
--
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
Re: [PR] feat(csharp/src/Drivers/BigQuery): Add support for AAD/Entra authentication [arrow-adbc]
davidhcoe commented on code in PR #2655:
URL: https://github.com/apache/arrow-adbc/pull/2655#discussion_r2052896656
##
csharp/src/Drivers/BigQuery/BigQueryParameters.cs:
##
@@ -22,6 +22,8 @@ namespace Apache.Arrow.Adbc.Drivers.BigQuery
///
public class BigQueryParameters
{
+public const string AccessToken = "adbc.bigquery.access_token";
+public const string AudienceUri = "adbc.bigquery.audience_uri";
Review Comment:
These could be used for any OAuth implementation but are only used for Entra
right now. I updated the readme.
--
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
Re: [PR] feat(csharp/src/Drivers/BigQuery): Add support for AAD/Entra authentication [arrow-adbc]
davidhcoe commented on code in PR #2655: URL: https://github.com/apache/arrow-adbc/pull/2655#discussion_r2052849677 ## csharp/src/Drivers/BigQuery/BigQueryParameters.cs: ## @@ -36,6 +38,8 @@ public class BigQueryParameters public const string Scopes = "adbc.bigquery.scopes"; public const string IncludeConstraintsWithGetObjects = "adbc.bigquery.include_constraints_getobjects"; public const string ClientTimeout = "adbc.bigquery.client.timeout"; +public const string MaximumRetryAttempts = "adbc.bigquery.maximum_retries"; +public const string RetryDelayMs = "adbc.bigquery.retry_delay_ms"; Review Comment: These are used to retry any error. -- 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
Re: [PR] feat(csharp/src/Drivers/BigQuery): Add support for AAD/Entra authentication [arrow-adbc]
CurtHagenlocher commented on code in PR #2655:
URL: https://github.com/apache/arrow-adbc/pull/2655#discussion_r2052539701
##
csharp/src/Drivers/BigQuery/BigQueryParameters.cs:
##
@@ -22,6 +22,8 @@ namespace Apache.Arrow.Adbc.Drivers.BigQuery
///
public class BigQueryParameters
{
+public const string AccessToken = "adbc.bigquery.access_token";
+public const string AudienceUri = "adbc.bigquery.audience_uri";
Review Comment:
Are these properties just for EntraId or can they also be used for Google
OAuth? If the former, should they be named in a way to make that clear?
Otherwise I might think I can supply a Google OAuth access token this way.
##
csharp/src/Drivers/BigQuery/BigQueryParameters.cs:
##
@@ -36,6 +38,8 @@ public class BigQueryParameters
public const string Scopes = "adbc.bigquery.scopes";
public const string IncludeConstraintsWithGetObjects =
"adbc.bigquery.include_constraints_getobjects";
public const string ClientTimeout = "adbc.bigquery.client.timeout";
+public const string MaximumRetryAttempts =
"adbc.bigquery.maximum_retries";
+public const string RetryDelayMs = "adbc.bigquery.retry_delay_ms";
Review Comment:
Is the "retry" here specific to refreshing the credential, and if so, should
the property name reflect that?
##
csharp/src/Drivers/BigQuery/BigQueryConnection.cs:
##
@@ -36,17 +37,15 @@ namespace Apache.Arrow.Adbc.Drivers.BigQuery
///
/// BigQuery-specific implementation of
///
-public class BigQueryConnection : AdbcConnection
+public class BigQueryConnection : AdbcConnection, ITokenProtectedResource
Review Comment:
I know it's a "preexisting condition" but In general we've made the
implementation types all be internal. Is there a specific API required on this
class that's not exposed through `ITokenProtectedResource`?
##
csharp/src/Drivers/BigQuery/BigQueryConnection.cs:
##
@@ -36,17 +37,15 @@ namespace Apache.Arrow.Adbc.Drivers.BigQuery
///
/// BigQuery-specific implementation of
///
-public class BigQueryConnection : AdbcConnection
+public class BigQueryConnection : AdbcConnection, ITokenProtectedResource
{
-readonly IReadOnlyDictionary properties;
-BigQueryClient? client;
-GoogleCredential? credential;
+IReadOnlyDictionary properties;
Review Comment:
I feel like there's a lot of unnecessary complexity around keeping this as
an `IReadOnlyDictionary`. If it were instead a `Dictionary`
then it could retain its `readonly`-ness and initialized only once in the
constructor, and we wouldn't need to make a copy of it every time `SetOption`
was called.
##
csharp/src/Drivers/BigQuery/ITokenProtectedResource.cs:
##
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+using System;
+using System.Threading.Tasks;
+
+namespace Apache.Arrow.Adbc.Drivers.BigQuery
+{
+///
+/// Common interface for a token protected resource.
+///
+public interface ITokenProtectedResource
+{
+///
+/// The function to call when updating the token.
+///
+public Func? UpdateToken { get; set; }
+
+///
+/// Determines the token needs to be updated.
+///
+/// The exception that occurs.
+/// True/False indicating a refresh is needed.
+public bool TokenRequiresUpdate(Exception ex);
Review Comment:
How is client code expected to use this?
##
csharp/src/Drivers/BigQuery/BigQueryParameters.cs:
##
@@ -50,10 +54,18 @@ public class BigQueryParameters
public class BigQueryConstants
{
public const string UserAuthenticationType = "user";
+public const string EntraIdAuthenticationType = "aad";
public const string ServiceAccountAuthenticationType = "service";
public const string TokenEndpoint =
"https://accounts.google.com/o/oauth2/token";;
public const string TreatLargeDecimalAsString = "true";
+// Entra ID / Azure AD constants
+public const string GrantType =
"urn:ietf:params:oauth:gra
Re: [PR] feat(csharp/src/Drivers/BigQuery): Add support for AAD/Entra authentication [arrow-adbc]
davidhcoe commented on PR #2655: URL: https://github.com/apache/arrow-adbc/pull/2655#issuecomment-2803043157 This can wait until https://github.com/apache/arrow-adbc/pull/2698 closes and then I incorporate those changes in this PR. -- 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
Re: [PR] feat(csharp/src/Drivers/BigQuery): Add support for AAD/Entra authentication [arrow-adbc]
davidhcoe commented on PR #2655: URL: https://github.com/apache/arrow-adbc/pull/2655#issuecomment-2803036926 Updated test results after refactoring the tests:  -- 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
