davidhcoe commented on code in PR #1323:
URL: https://github.com/apache/arrow-adbc/pull/1323#discussion_r1409719891
##########
csharp/test/Drivers/Snowflake/ClientTests.cs:
##########
@@ -228,22 +228,37 @@ private Adbc.Client.AdbcConnection
GetSnowflakeAdbcConnectionUsingConnectionStri
builder[SnowflakeParameters.HOST] = testConfiguration.Host;
builder[SnowflakeParameters.DATABASE] = testConfiguration.Database;
builder[SnowflakeParameters.USERNAME] = testConfiguration.User;
- if
(!string.IsNullOrEmpty(testConfiguration.AuthenticationTokenPath))
+ if (authType == SnowflakeAuthentication.AuthJwt)
{
- builder[SnowflakeParameters.AUTH_TYPE] =
testConfiguration.AuthenticationType;
- string privateKey =
File.ReadAllText(testConfiguration.AuthenticationTokenPath);
- if (testConfiguration.AuthenticationType.Equals("auth_jwt",
StringComparison.OrdinalIgnoreCase))
+ string privateKeyFile =
testConfiguration.Authentication.SnowflakeJwt.PrivateKeyFile;
+ string privateKey =
testConfiguration.Authentication.SnowflakeJwt.PrivateKey;
+
+ if (privateKeyFile != null)
{
- builder[SnowflakeParameters.PKCS8_VALUE] = privateKey;
- if(!string.IsNullOrEmpty(testConfiguration.Pkcs8Passcode))
- {
- builder[SnowflakeParameters.PKCS8_PASS] =
testConfiguration.Pkcs8Passcode;
- }
+ privateKey = File.ReadAllText(privateKeyFile);
+ }
+ builder[SnowflakeParameters.AUTH_TYPE] =
SnowflakeAuthentication.AuthJwt;
+ builder[SnowflakeParameters.PKCS8_VALUE] = privateKey;
+ builder[SnowflakeParameters.USERNAME] =
testConfiguration.Authentication.SnowflakeJwt.User;
+ if
(!string.IsNullOrEmpty(testConfiguration.Authentication.SnowflakeJwt.PrivateKeyPassPhrase))
+ {
+ builder[SnowflakeParameters.PKCS8_PASS] =
testConfiguration.Authentication.SnowflakeJwt.PrivateKeyPassPhrase;
+ }
+ }
+ else if (authType == SnowflakeAuthentication.AuthOAuth)
+ {
+ builder[SnowflakeParameters.AUTH_TYPE] =
SnowflakeAuthentication.AuthOAuth;
+ builder[SnowflakeParameters.AUTH_TOKEN] =
testConfiguration.Authentication.OAuth.Token;
+ if (testConfiguration.Authentication.OAuth.User != null)
+ {
+ builder[SnowflakeParameters.USERNAME] =
testConfiguration.Authentication.OAuth.User;
}
}
else
{
- builder[SnowflakeParameters.PASSWORD] =
testConfiguration.Password;
+ // default basic auth
Review Comment:
This needs to specify `auth_snowflake` (what is currently called Default)
##########
csharp/test/Drivers/Snowflake/ClientTests.cs:
##########
@@ -228,22 +228,37 @@ private Adbc.Client.AdbcConnection
GetSnowflakeAdbcConnectionUsingConnectionStri
builder[SnowflakeParameters.HOST] = testConfiguration.Host;
builder[SnowflakeParameters.DATABASE] = testConfiguration.Database;
builder[SnowflakeParameters.USERNAME] = testConfiguration.User;
- if
(!string.IsNullOrEmpty(testConfiguration.AuthenticationTokenPath))
+ if (authType == SnowflakeAuthentication.AuthJwt)
{
- builder[SnowflakeParameters.AUTH_TYPE] =
testConfiguration.AuthenticationType;
- string privateKey =
File.ReadAllText(testConfiguration.AuthenticationTokenPath);
- if (testConfiguration.AuthenticationType.Equals("auth_jwt",
StringComparison.OrdinalIgnoreCase))
+ string privateKeyFile =
testConfiguration.Authentication.SnowflakeJwt.PrivateKeyFile;
+ string privateKey =
testConfiguration.Authentication.SnowflakeJwt.PrivateKey;
+
+ if (privateKeyFile != null)
{
- builder[SnowflakeParameters.PKCS8_VALUE] = privateKey;
- if(!string.IsNullOrEmpty(testConfiguration.Pkcs8Passcode))
- {
- builder[SnowflakeParameters.PKCS8_PASS] =
testConfiguration.Pkcs8Passcode;
- }
+ privateKey = File.ReadAllText(privateKeyFile);
+ }
+ builder[SnowflakeParameters.AUTH_TYPE] =
SnowflakeAuthentication.AuthJwt;
Review Comment:
should this just take the parameter value?
##########
csharp/test/Drivers/Snowflake/Resources/snowflakeconfig.json:
##########
@@ -4,19 +4,28 @@
"account": "",
"host": "",
"database": "",
- "user": "",
- "password": "",
"warehouse": "",
- "authenticationType": "",
- "authenticationTokenPath": "",
- "pkcs8Passcode": "",
- "useHighPrecision": true,
+ "useHighPrecision": true,
"metadata": {
"catalog": "",
"schema": "",
"table": "",
"expectedColumnCount": 0
},
+ "authentication": {
+ "auth_oauth": {
+ "token": ""
+ },
+ "auth_jwt": {
+ "private_key_file": "",
+ "private_key_pwd": "",
+ "user": ""
+ },
+ "default": {
Review Comment:
"default" is not an auth type. This should be `auth_snowflake`
##########
csharp/test/Drivers/Snowflake/SnowflakeTestConfiguration.cs:
##########
@@ -79,21 +79,65 @@ internal class SnowflakeTestConfiguration :
TestConfiguration
public string AuthenticationType { get; set; }
Review Comment:
This can be removed
##########
csharp/test/Drivers/Snowflake/SnowflakeTestConfiguration.cs:
##########
@@ -79,21 +79,65 @@ internal class SnowflakeTestConfiguration :
TestConfiguration
public string AuthenticationType { get; set; }
/// <summary>
- /// The file location of the authentication token (if using).
+ /// The Snowflake use high precision
/// </summary>
- [JsonPropertyName("authenticationTokenPath"), JsonIgnore(Condition =
JsonIgnoreCondition.WhenWritingDefault)]
- public string AuthenticationTokenPath { get; set; }
+ [JsonPropertyName("useHighPrecision")]
+ public bool UseHighPrecision { get; set; } = true;
/// <summary>
- /// The passcode to use if the JWT token is encrypted.
+ /// The snowflake Authentication
/// </summary>
- [JsonPropertyName("pkcs8Passcode"), JsonIgnore(Condition =
JsonIgnoreCondition.WhenWritingDefault)]
- public string Pkcs8Passcode { get; set; }
+ [JsonPropertyName("authentication")]
+ public SnowflakeAuthentication Authentication { get; set; }
- /// <summary>
- /// The Snowflake authentication type.
- /// </summary>
- [JsonPropertyName("useHighPrecision")]
- public bool UseHighPrecision { get; set; } = true;
+ }
+
+ public class SnowflakeAuthentication
+ {
+ public const string AuthOAuth = "auth_oauth";
+ public const string AuthJwt = "auth_jwt";
+ public const string AuthDefault = "default";
Review Comment:
This should be auth_snowflake
##########
csharp/test/Drivers/Snowflake/ClientTests.cs:
##########
@@ -151,7 +151,7 @@ public void CanClientExecuteQueryUsingPrivateKey()
{
SnowflakeTestConfiguration testConfiguration =
Utils.LoadTestConfiguration<SnowflakeTestConfiguration>(SnowflakeTestingUtils.SNOWFLAKE_TEST_CONFIG_VARIABLE);
Review Comment:
This will fail if the configuration is not using JWT. Here is a suggested
change:
```
Skip.If(testConfiguration.Authentication.SnowflakeJwt is null, "JWT
authentication is not configured");
```
##########
csharp/test/Drivers/Snowflake/ClientTests.cs:
##########
@@ -166,7 +166,7 @@ public void VerifyTypesAndValues()
{
SnowflakeTestConfiguration testConfiguration =
Utils.LoadTestConfiguration<SnowflakeTestConfiguration>(SnowflakeTestingUtils.SNOWFLAKE_TEST_CONFIG_VARIABLE);
- using (Adbc.Client.AdbcConnection adbcConnection =
GetSnowflakeAdbcConnection(testConfiguration))
+ using (Adbc.Client.AdbcConnection adbcConnection =
GetSnowflakeAdbcConnectionUsingConnectionString(testConfiguration))
Review Comment:
All of the changes to `GetSnowflakeAdbcConnectionUsingConnectionString` make
`GetSnowflakeAdbcConnection` an obsolute method that can be removed.
##########
csharp/test/Drivers/Snowflake/SnowflakeTestingUtils.cs:
##########
@@ -62,7 +63,6 @@ internal class SnowflakeTestingUtils
{ SnowflakeParameters.USERNAME, testConfiguration.User },
{ SnowflakeParameters.PASSWORD, testConfiguration.Password },
{ SnowflakeParameters.WAREHOUSE, testConfiguration.Warehouse },
- { SnowflakeParameters.AUTH_TYPE,
testConfiguration.AuthenticationType },
Review Comment:
this causes a failure in the DriverTests. A suggested change is:
```
parameters = new Dictionary<string, string>
{
{ SnowflakeParameters.ACCOUNT, testConfiguration.Account},
{ SnowflakeParameters.WAREHOUSE, testConfiguration.Warehouse
},
{ SnowflakeParameters.USE_HIGH_PRECISION,
testConfiguration.UseHighPrecision.ToString().ToLowerInvariant() }
};
if(testConfiguration.Authentication.Default is not null)
{
parameters[SnowflakeParameters.AUTH_TYPE] =
SnowflakeAuthentication.AuthDefault;
parameters[SnowflakeParameters.USERNAME] =
testConfiguration.Authentication.Default.User;
parameters[SnowflakeParameters.PASSWORD] =
testConfiguration.Authentication.Default.Password;
}
```
--
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]
