Pass connection string as parameter instead of reading from environment

[sgaunet]

Problem

The tryConnect() method in internal/app/app.go (lines 286-289) reads the connection string directly from environment variables using os.Getenv(), making the code harder to test and less flexible.

Current implementation:

func (a *App) tryConnect() error {
    connectionString := os.Getenv("POSTGRES_URL")
    if connectionString == "" {
        connectionString = os.Getenv("DATABASE_URL")
    }
    
    if connectionString == "" {
        return ErrNoConnectionString
    }
    
    // ... connect logic ...
}

Issues

1. Hard to test: Tests can't override connection string without modifying environment
2. Hidden dependency: Function signature doesn't show it needs environment variables
3. Global state: Relies on global os.Getenv() instead of explicit parameters
4. Race conditions: Tests modifying env vars can interfere with each other
5. Not explicit: Callers don't know where connection string comes from

Current Workarounds

Tests must set environment variables:

func TestConnect(t *testing.T) {
    os.Setenv("POSTGRES_URL", "postgres://...")
    defer os.Unsetenv("POSTGRES_URL")
    
    app := New()
    err := app.Connect("")
    // ...
}

This is fragile and can cause test pollution.

Proposed Solution

Option 1: Accept Connection String as Parameter (Recommended)

func (a *App) Connect(connectionString string) error {
    if err := a.ensureConnection(); err != nil {
        return err
    }
    return nil
}

func (a *App) tryConnect(connectionString string) error {
    if connectionString == "" {
        return ErrNoConnectionString
    }
    
    client, err := NewPostgreSQLClient(connectionString)
    if err != nil {
        return fmt.Errorf("failed to create client: %w", err)
    }
    
    a.client = client
    return nil
}

Then in main.go, read environment variables at the top level:

func main() {
    connectionString := os.Getenv("POSTGRES_URL")
    if connectionString == "" {
        connectionString = os.Getenv("DATABASE_URL")
    }
    
    app := app.New()
    if connectionString != "" {
        if err := app.Connect(connectionString); err != nil {
            logger.Log().Error().Err(err).Msg("Failed to connect")
        }
    }
    // ...
}

Benefits:

• Environment variable logic centralized in main()
• App methods don't depend on global state
• Easy to test with different connection strings
• Explicit about where connection string comes from

Option 2: Configuration Struct

type AppConfig struct {
    ConnectionString string
}

func NewWithConfig(client PostgreSQLClient, config AppConfig) *App {
    return &App{
        client: client,
        config: config,
    }
}

func (a *App) Connect() error {
    if a.config.ConnectionString == "" {
        return ErrNoConnectionString
    }
    return a.tryConnect(a.config.ConnectionString)
}

Option 3: Dependency Injection for Config Provider

type ConfigProvider interface {
    GetConnectionString() string
}

type EnvConfigProvider struct{}

func (e *EnvConfigProvider) GetConnectionString() string {
    if s := os.Getenv("POSTGRES_URL"); s != "" {
        return s
    }
    return os.Getenv("DATABASE_URL")
}

// In tests
type TestConfigProvider struct {
    connectionString string
}

func (t *TestConfigProvider) GetConnectionString() string {
    return t.connectionString
}

Recommended Approach

Use Option 1 (pass as parameter) because:

• Simplest solution
• Clear dependency
• Easy to test
• Follows Go best practices for avoiding global state

Migration Impact

Code Changes Required

App layer (internal/app/app.go):

• Update tryConnect() to accept connection string parameter
• Update Connect() to accept and pass connection string

Main layer (main.go):

• Read environment variables at startup
• Pass connection string to Connect() method

Tests:

• Pass connection strings directly instead of setting env vars
• Remove os.Setenv / os.Unsetenv pairs

Backwards Compatibility

This is a breaking change to the internal API, but since this is internal code, it's acceptable.

Impact

• Severity: LOW
• Type: Refactoring, Testing
• Location: internal/app/app.go:286-289
• Benefits: Better testability, explicit dependencies, no global state

Checklist

• Update tryConnect() to accept connectionString parameter
• Update Connect() to accept connectionString parameter
• Move environment variable reading to main.go
• Update all tests to pass connection strings directly
• Remove os.Setenv usage from tests
• Update CLAUDE.md if it mentions environment variable handling
• Consider adding validation for connection string format
• Document connection string parameter in GoDoc comments