Skip to content

Commit d810d21

Browse files
cevianclaude
andcommitted
Fix db test-connection command to work with keyring password storage
- Add buildConnectionConfig() function to handle pgx config with password retrieval - Refactor testDatabaseConnection() to use buildConnectionConfig() for better testability - Set config.Password when using keyring storage method for pgx authentication - Preserve existing pgpass behavior (pgx automatically checks ~/.pgpass file) - Update function signatures to pass service parameter for password retrieval - Add comprehensive tests that verify password is actually set in pgx config: - TestBuildConnectionConfig_KeyringPassword: Stores real password in keyring and verifies config.Password is set - TestBuildConnectionConfig_PgpassStorage_NoPasswordSet: Verifies no password set for pgpass method - Update existing tests with service parameter for compatibility - Follow same pattern as db psql fix for consistency The db test-connection command now works properly with all password storage methods: - keyring: Retrieves password from keyring and sets config.Password for pgx - pgpass: Lets pgx automatically read ~/.pgpass file (unchanged) - none: Uses interactive prompts or existing environment variables (unchanged) Tests actually verify that password retrieval and config setting works correctly. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
1 parent bba9797 commit d810d21

File tree

2 files changed

+110
-7
lines changed

2 files changed

+110
-7
lines changed

internal/tiger/cmd/db.go

Lines changed: 27 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -194,7 +194,7 @@ Examples:
194194
}
195195

196196
// Test the connection
197-
return testDatabaseConnection(connectionString, dbTestConnectionTimeout, cmd)
197+
return testDatabaseConnection(connectionString, dbTestConnectionTimeout, service, cmd)
198198
},
199199
}
200200

@@ -382,8 +382,30 @@ func buildPsqlCommand(connectionString, psqlPath string, additionalFlags []strin
382382
return psqlCmd
383383
}
384384

385+
// buildConnectionConfig creates a pgx connection config with proper password handling
386+
func buildConnectionConfig(connectionString string, service api.Service) (*pgx.ConnConfig, error) {
387+
// Parse the connection string first to validate it
388+
config, err := pgx.ParseConfig(connectionString)
389+
if err != nil {
390+
return nil, err
391+
}
392+
393+
// Set password from keyring storage if available
394+
// pgpass storage works automatically since pgx checks ~/.pgpass file
395+
storage := GetPasswordStorage()
396+
if _, isKeyring := storage.(*KeyringStorage); isKeyring {
397+
if password, err := storage.Get(service); err == nil && password != "" {
398+
config.Password = password
399+
}
400+
// Note: If keyring password retrieval fails, we let pgx try without it
401+
// This allows fallback to other authentication methods
402+
}
403+
404+
return config, nil
405+
}
406+
385407
// testDatabaseConnection tests the database connection and returns appropriate exit codes
386-
func testDatabaseConnection(connectionString string, timeout time.Duration, cmd *cobra.Command) error {
408+
func testDatabaseConnection(connectionString string, timeout time.Duration, service api.Service, cmd *cobra.Command) error {
387409
// Create context with timeout if specified
388410
var ctx context.Context
389411
var cancel context.CancelFunc
@@ -395,10 +417,10 @@ func testDatabaseConnection(connectionString string, timeout time.Duration, cmd
395417
ctx = context.Background()
396418
}
397419

398-
// Parse the connection string first to validate it
399-
config, err := pgx.ParseConfig(connectionString)
420+
// Build connection config with proper password handling
421+
config, err := buildConnectionConfig(connectionString, service)
400422
if err != nil {
401-
fmt.Fprintf(cmd.ErrOrStderr(), "Failed to parse connection string: %v\n", err)
423+
fmt.Fprintf(cmd.ErrOrStderr(), "Failed to build connection config: %v\n", err)
402424
return exitWithCode(3, err) // Invalid parameters
403425
}
404426

internal/tiger/cmd/db_test.go

Lines changed: 83 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -417,6 +417,85 @@ func TestBuildPsqlCommand_PgpassStorage_NoEnvVar(t *testing.T) {
417417
}
418418
}
419419

420+
func TestBuildConnectionConfig_KeyringPassword(t *testing.T) {
421+
// This test verifies that buildConnectionConfig properly sets password from keyring
422+
423+
// Set keyring as the password storage method for this test
424+
originalStorage := viper.GetString("password_storage")
425+
viper.Set("password_storage", "keyring")
426+
defer viper.Set("password_storage", originalStorage)
427+
428+
// Create a test service
429+
serviceID := "test-connection-config-service"
430+
projectID := "test-connection-config-project"
431+
service := api.Service{
432+
ServiceId: &serviceID,
433+
ProjectId: &projectID,
434+
}
435+
436+
// Store a test password in keyring
437+
testPassword := "test-connection-config-password-789"
438+
storage := GetPasswordStorage()
439+
err := storage.Save(service, testPassword)
440+
if err != nil {
441+
t.Fatalf("Failed to save test password: %v", err)
442+
}
443+
defer storage.Remove(service) // Clean up after test
444+
445+
connectionString := "postgresql://testuser@testhost:5432/testdb?sslmode=require"
446+
447+
// Call the actual production function that builds the config
448+
config, err := buildConnectionConfig(connectionString, service)
449+
450+
if err != nil {
451+
t.Fatalf("buildConnectionConfig failed: %v", err)
452+
}
453+
454+
if config == nil {
455+
t.Fatal("buildConnectionConfig returned nil config")
456+
}
457+
458+
// Verify that the password was set in the config
459+
if config.Password != testPassword {
460+
t.Errorf("Expected password '%s' to be set in config, but got '%s'", testPassword, config.Password)
461+
}
462+
}
463+
464+
func TestBuildConnectionConfig_PgpassStorage_NoPasswordSet(t *testing.T) {
465+
// This test verifies that buildConnectionConfig doesn't set password for pgpass storage
466+
467+
// Set pgpass as the password storage method for this test
468+
originalStorage := viper.GetString("password_storage")
469+
viper.Set("password_storage", "pgpass")
470+
defer viper.Set("password_storage", originalStorage)
471+
472+
// Create a test service
473+
serviceID := "test-connection-config-pgpass"
474+
projectID := "test-connection-config-project"
475+
service := api.Service{
476+
ServiceId: &serviceID,
477+
ProjectId: &projectID,
478+
}
479+
480+
connectionString := "postgresql://testuser@testhost:5432/testdb?sslmode=require"
481+
482+
// Call the actual production function that builds the config
483+
config, err := buildConnectionConfig(connectionString, service)
484+
485+
if err != nil {
486+
t.Fatalf("buildConnectionConfig failed: %v", err)
487+
}
488+
489+
if config == nil {
490+
t.Fatal("buildConnectionConfig returned nil config")
491+
}
492+
493+
// Verify that no password was set in the config (pgx will check ~/.pgpass automatically)
494+
if config.Password != "" {
495+
t.Errorf("Expected no password to be set in config for pgpass storage, but got '%s'", config.Password)
496+
}
497+
}
498+
420499
func TestSeparateServiceAndPsqlArgs(t *testing.T) {
421500
testCases := []struct {
422501
name string
@@ -582,7 +661,8 @@ func TestTestDatabaseConnection_InvalidConnectionString(t *testing.T) {
582661

583662
// Test with malformed connection string (should return exit code 3)
584663
invalidConnectionString := "this is not a valid connection string at all"
585-
err := testDatabaseConnection(invalidConnectionString, 1, cmd)
664+
service := api.Service{} // Dummy service for test
665+
err := testDatabaseConnection(invalidConnectionString, 1, service, cmd)
586666

587667
if err == nil {
588668
t.Error("Expected error for invalid connection string")
@@ -610,8 +690,9 @@ func TestTestDatabaseConnection_Timeout(t *testing.T) {
610690
// Use a connection string to a non-routable IP to test timeout
611691
timeoutConnectionString := "postgresql://user:pass@192.0.2.1:5432/db?sslmode=disable&connect_timeout=1"
612692

693+
service := api.Service{} // Dummy service for test
613694
start := time.Now()
614-
err := testDatabaseConnection(timeoutConnectionString, 1, cmd) // 1 second timeout
695+
err := testDatabaseConnection(timeoutConnectionString, 1, service, cmd) // 1 second timeout
615696
duration := time.Since(start)
616697

617698
if err == nil {

0 commit comments

Comments
 (0)