lidavidm commented on code in PR #3174:
URL: https://github.com/apache/arrow-adbc/pull/3174#discussion_r2218222000
##########
go/adbc/driver/bigquery/driver_test.go:
##########
@@ -1551,3 +1551,73 @@ func (suite *BigQueryTests)
TestMetadataGetObjectsColumnsXdbc() {
}
var _ validation.DriverQuirks = (*BigQueryQuirks)(nil)
+
+// TestDriverConstructorWithNilClientFactory tests that the driver constructor
+// handles nil client factory gracefully by using the default factory.
+func TestDriverConstructorWithNilClientFactory(t *testing.T) {
+ mem := memory.NewCheckedAllocator(memory.DefaultAllocator)
+ defer mem.AssertSize(t, 0)
+
+ // Test that NewDriverWithClientFactory with nil doesn't panic
+ drv := driver.NewDriverWithClientFactory(mem, nil)
+ if drv == nil {
+ t.Error("Driver should not be nil")
+ }
+
+ // Test that we can create a database with the driver
+ db, err := drv.NewDatabase(nil)
+ if err != nil {
+ t.Errorf("Failed to create database: %v", err)
+ }
+ if db == nil {
+ t.Error("Database should not be nil")
+ }
+ defer db.Close()
+}
+
+// TestWithAccessTokenConstant tests that the WithAccessToken constant
+// follows the correct BigQuery naming pattern.
+func TestWithAccessTokenConstant(t *testing.T) {
+ expected := "adbc.google.bigquery.auth.access_token"
+ if driver.WithAccessToken != expected {
+ t.Errorf("WithAccessToken constant should be %s, got %s",
expected, driver.WithAccessToken)
+ }
+}
Review Comment:
nit, but is this kind of test really necessary...?
##########
go/adbc/driver/bigquery/connection.go:
##########
@@ -470,6 +509,32 @@ func (c *connectionImpl) GetOption(key string) (string,
error) {
}
}
+func (c *connectionImpl) SetOption(key string, value string) error {
+ switch key {
+ case OptionStringAuthType:
+ c.authType = value
+ case OptionStringAuthCredentials:
+ c.credentials = value
+ case OptionStringAuthClientID:
+ c.clientID = value
+ case OptionStringAuthClientSecret:
+ c.clientSecret = value
+ case OptionStringAuthRefreshToken:
+ c.refreshToken = value
+ case WithImpersonateTargetPrincipal:
Review Comment:
Can these constants be renamed to follow the existing naming convention?
##########
go/adbc/driver/bigquery/driver_test.go:
##########
@@ -1551,3 +1551,73 @@ func (suite *BigQueryTests)
TestMetadataGetObjectsColumnsXdbc() {
}
var _ validation.DriverQuirks = (*BigQueryQuirks)(nil)
+
+// TestDriverConstructorWithNilClientFactory tests that the driver constructor
+// handles nil client factory gracefully by using the default factory.
+func TestDriverConstructorWithNilClientFactory(t *testing.T) {
+ mem := memory.NewCheckedAllocator(memory.DefaultAllocator)
+ defer mem.AssertSize(t, 0)
+
+ // Test that NewDriverWithClientFactory with nil doesn't panic
+ drv := driver.NewDriverWithClientFactory(mem, nil)
+ if drv == nil {
+ t.Error("Driver should not be nil")
+ }
+
+ // Test that we can create a database with the driver
+ db, err := drv.NewDatabase(nil)
+ if err != nil {
+ t.Errorf("Failed to create database: %v", err)
+ }
+ if db == nil {
+ t.Error("Database should not be nil")
+ }
+ defer db.Close()
+}
+
+// TestWithAccessTokenConstant tests that the WithAccessToken constant
+// follows the correct BigQuery naming pattern.
+func TestWithAccessTokenConstant(t *testing.T) {
+ expected := "adbc.google.bigquery.auth.access_token"
+ if driver.WithAccessToken != expected {
+ t.Errorf("WithAccessToken constant should be %s, got %s",
expected, driver.WithAccessToken)
+ }
+}
+
+// TestAuthTypeConsolidation tests that all auth type values are handled
+// correctly in the consolidated switch statement.
+func TestAuthTypeConsolidation(t *testing.T) {
+ mem := memory.NewCheckedAllocator(memory.DefaultAllocator)
+ defer mem.AssertSize(t, 0)
+
+ drv := driver.NewDriver(mem)
+ db, err := drv.NewDatabase(nil)
+ if err != nil {
+ t.Fatalf("Failed to create database: %v", err)
+ }
+ defer db.Close()
Review Comment:
This needs to be `defer CheckedClose(...)` as `Close` is fallible.
##########
go/adbc/driver/bigquery/driver_test.go:
##########
@@ -1551,3 +1551,73 @@ func (suite *BigQueryTests)
TestMetadataGetObjectsColumnsXdbc() {
}
var _ validation.DriverQuirks = (*BigQueryQuirks)(nil)
+
+// TestDriverConstructorWithNilClientFactory tests that the driver constructor
+// handles nil client factory gracefully by using the default factory.
+func TestDriverConstructorWithNilClientFactory(t *testing.T) {
+ mem := memory.NewCheckedAllocator(memory.DefaultAllocator)
+ defer mem.AssertSize(t, 0)
+
+ // Test that NewDriverWithClientFactory with nil doesn't panic
+ drv := driver.NewDriverWithClientFactory(mem, nil)
+ if drv == nil {
+ t.Error("Driver should not be nil")
+ }
Review Comment:
Use `assert.Errorf` etc?
##########
go/adbc/driver/bigquery/driver.go:
##########
@@ -77,6 +77,49 @@ const (
AccessTokenEndpoint = "https://accounts.google.com/o/oauth2/token";
AccessTokenServerName = "google.com"
+
+ // UseAppDefaultCredentials instructs the driver to authenticate using
+ // Application Default Credentials (ADC).
+ OptionValueAuthTypeAppDefaultCredentials = "app_default_credentials"
+)
+
+// option that can be passed to the NewClient method.
+const (
+ // WithAccessToken instructs the driver to authenticate using the given
+ // access token.
+ WithAccessToken = "adbc.google.bigquery.auth.access_token"
Review Comment:
Where is this actually used in the driver?
--
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
