diff --git a/.claude/golang-expert/testing-strategy.md b/.claude/golang-expert/testing-strategy.md index 49ba4faf..26a537a3 100644 --- a/.claude/golang-expert/testing-strategy.md +++ b/.claude/golang-expert/testing-strategy.md @@ -307,6 +307,38 @@ func skipIfNoDatabase(t *testing.T) *pgxpool.Pool { } ``` +### Testing Command-Line Flag Precedence + +Configuration is layered as defaults, then configuration file, then +command-line flags. Whether a flag overrides the file is decided by +whether the operator actually passed it, never by comparing the +flag's value against its default; a value comparison silently drops +an explicitly passed default value and clobbers a file value that +happens to match a default. + +The shared helper `pkg/flagutil` provides `Passed(fs *flag.FlagSet) +Set`, backed by `flag.FlagSet.Visit`, and `Set.Has(name)`. The +collector threads a `flagutil.Set` into `loadConfiguration` and +`(*Config).ApplyFlags`; the alerter carries one in the `Passed` +field of its `flagOverrides` struct, which `applyFlagOverrides` and +the SIGHUP reload path both consume. Both binaries declare their +flag names as constants, so registration and lookup cannot drift. + +Tests construct the set directly rather than parsing a command +line, so both branches of every override are reachable: + +```go +// The flag was passed, carrying the value that is also its default. +cfg.ApplyFlags(flagutil.Set{flagPGPort: true}) + +// Nothing was passed, so the config file value must survive. +cfg.ApplyFlags(nil) +``` + +Any new flag needs both cases covered: a configuration value that +coincides with the flag's default must survive when the flag is +absent, and an explicitly passed default value must still apply. + ## Database Testing ### Test Database Lifecycle diff --git a/alerter/src/cmd/ai-dba-alerter/flagoverrides_test.go b/alerter/src/cmd/ai-dba-alerter/flagoverrides_test.go new file mode 100644 index 00000000..679deb95 --- /dev/null +++ b/alerter/src/cmd/ai-dba-alerter/flagoverrides_test.go @@ -0,0 +1,135 @@ +/*------------------------------------------------------------------------- + * + * pgEdge AI DBA Workbench + * + * Copyright (c) 2025 - 2026, pgEdge, Inc. + * This software is released under The PostgreSQL License + * + *------------------------------------------------------------------------- + */ +package main + +import ( + "os" + "path/filepath" + "testing" + + "github.com/pgedge/ai-workbench/alerter/internal/config" + "github.com/pgedge/ai-workbench/pkg/flagutil" +) + +// TestApplyFlagOverrides_ExplicitDefaultValueWins covers the case the +// old value-comparison logic could not express: the operator passes a +// flag whose value equals the flag's registered default, and the flag +// must still override the configuration file. +func TestApplyFlagOverrides_ExplicitDefaultValueWins(t *testing.T) { + cfg := config.NewConfig() + // Values as if they had come from a configuration file. + cfg.Datastore.Host = "file-host" + cfg.Datastore.Port = 6000 + cfg.Datastore.SSLMode = "require" + + // Every value below is the corresponding flag's registered + // default, yet each flag was explicitly passed, so each must + // still be applied. + err := applyFlagOverrides(cfg, flagOverrides{ + DBHost: "", + DBPort: 0, + DBSSLMode: "", + Passed: flagutil.Set{ + flagDBHost: true, + flagDBPort: true, + flagDBSSLMode: true, + }, + }) + if err != nil { + t.Fatalf("applyFlagOverrides: %v", err) + } + if cfg.Datastore.Host != "" { + t.Errorf("Host = %q, want the explicitly passed empty value", cfg.Datastore.Host) + } + if cfg.Datastore.Port != 0 { + t.Errorf("Port = %d, want the explicitly passed 0", cfg.Datastore.Port) + } + if cfg.Datastore.SSLMode != "" { + t.Errorf("SSLMode = %q, want the explicitly passed empty value", cfg.Datastore.SSLMode) + } +} + +// TestApplyFlagOverrides_ConfigMatchingFlagDefaultSurvives is the +// mirror image: configuration values that coincide with the flags' +// registered defaults must survive when no flag was passed. +func TestApplyFlagOverrides_ConfigMatchingFlagDefaultSurvives(t *testing.T) { + cfg := config.NewConfig() + cfg.Datastore.Host = "" + cfg.Datastore.Port = 0 + cfg.Datastore.SSLMode = "" + + if err := applyFlagOverrides(cfg, flagOverrides{ + DBHost: "unused-host", + DBPort: 9999, + DBSSLMode: "unused-mode", + }); err != nil { + t.Fatalf("applyFlagOverrides: %v", err) + } + if cfg.Datastore.Host != "" || cfg.Datastore.Port != 0 || + cfg.Datastore.SSLMode != "" { + t.Errorf("unpassed flags overrode the config: %+v", cfg.Datastore) + } +} + +// TestApplyFlagOverrides_ExplicitEmptyPasswordFile verifies that an +// explicitly empty -db-password-file clears the path the +// configuration file named, rather than being read as a path (which +// would fail) or silently leaving the configured file in place for +// cfg.LoadPassword to read later. +func TestApplyFlagOverrides_ExplicitEmptyPasswordFile(t *testing.T) { + cfg := config.NewConfig() + cfg.Datastore.PasswordFile = "/from/config/password" + + if err := applyFlagOverrides(cfg, flagOverrides{ + DBPasswordFile: "", + Passed: flagutil.Set{flagDBPasswordFile: true}, + }); err != nil { + t.Fatalf("applyFlagOverrides: %v", err) + } + if cfg.Datastore.PasswordFile != "" { + t.Errorf("PasswordFile = %q, want it cleared by the explicit empty flag", + cfg.Datastore.PasswordFile) + } + + // The configured file must not be read on a later LoadPassword + // either; it does not exist, so a read attempt would error. + if err := cfg.LoadPassword(); err != nil { + t.Errorf("LoadPassword after clearing the path: %v", err) + } + if cfg.Datastore.Password != "" { + t.Errorf("Password = %q, want it left empty", cfg.Datastore.Password) + } +} + +// TestApplyFlagOverrides_PasswordFileReplacesConfigured verifies that +// a non-empty -db-password-file both loads the password and replaces +// the path the configuration file named. +func TestApplyFlagOverrides_PasswordFileReplacesConfigured(t *testing.T) { + pwFile := filepath.Join(t.TempDir(), "pw.txt") + if err := os.WriteFile(pwFile, []byte("flag-password\n"), 0600); err != nil { + t.Fatalf("write: %v", err) + } + + cfg := config.NewConfig() + cfg.Datastore.PasswordFile = "/from/config/password" + + if err := applyFlagOverrides(cfg, flagOverrides{ + DBPasswordFile: pwFile, + Passed: flagutil.Set{flagDBPasswordFile: true}, + }); err != nil { + t.Fatalf("applyFlagOverrides: %v", err) + } + if cfg.Datastore.PasswordFile != pwFile { + t.Errorf("PasswordFile = %q, want %q", cfg.Datastore.PasswordFile, pwFile) + } + if cfg.Datastore.Password != "flag-password" { + t.Errorf("Password = %q, want flag-password", cfg.Datastore.Password) + } +} diff --git a/alerter/src/cmd/ai-dba-alerter/main.go b/alerter/src/cmd/ai-dba-alerter/main.go index 77d251e8..4a227357 100644 --- a/alerter/src/cmd/ai-dba-alerter/main.go +++ b/alerter/src/cmd/ai-dba-alerter/main.go @@ -22,6 +22,7 @@ import ( "github.com/pgedge/ai-workbench/alerter/internal/database" "github.com/pgedge/ai-workbench/alerter/internal/engine" "github.com/pgedge/ai-workbench/pkg/fileutil" + "github.com/pgedge/ai-workbench/pkg/flagutil" ) // Version information @@ -60,17 +61,37 @@ func resolveConfigPath(flagValue string) resolveConfigPathResult { } } -// reloadFlagOverrides bundles the CLI flag values that survive -// across a SIGHUP reload and must be reapplied to the freshly -// loaded config so operators do not lose their command-line -// overrides on every reload. -type reloadFlagOverrides struct { +// Names of the database connection flags. They are named constants +// so the registration in main and the override logic in +// applyFlagOverrides cannot drift apart; the override logic looks a +// flag up by name to decide whether the operator actually passed it. +const ( + flagDBHost = "db-host" + flagDBPort = "db-port" + flagDBName = "db-name" + flagDBUser = "db-user" + flagDBPasswordFile = "db-password-file" + flagDBSSLMode = "db-sslmode" +) + +// flagOverrides bundles the CLI flag values that override the +// configuration file, together with the set of flags the operator +// actually passed. The same bundle is reapplied to the freshly +// loaded config across a SIGHUP reload so operators do not lose +// their command-line overrides on every reload. +// +// Passed is what makes an override unambiguous: a flag value on its +// own cannot distinguish "the operator asked for this" from "this is +// the registered default", so a config file value that happens to +// match a default would otherwise be overwritten. +type flagOverrides struct { DBHost string DBPort int DBName string DBUser string DBPasswordFile string DBSSLMode string + Passed flagutil.Set } // reloadConfigOnSignal builds a fresh *config.Config from the same @@ -95,7 +116,7 @@ func reloadConfigOnSignal( logOut io.Writer, prevPath string, explicit bool, - overrides reloadFlagOverrides, + overrides flagOverrides, ) (*config.Config, error) { reloadPath := prevPath if !explicit { @@ -119,10 +140,7 @@ func reloadConfigOnSignal( return nil, fmt.Errorf("failed to reload config: %w", err) } - if err := applyFlagOverrides(newCfg, - overrides.DBHost, overrides.DBPort, overrides.DBName, - overrides.DBUser, overrides.DBPasswordFile, overrides.DBSSLMode, - ); err != nil { + if err := applyFlagOverrides(newCfg, overrides); err != nil { return nil, fmt.Errorf("failed to apply overrides on reload: %w", err) } @@ -156,15 +174,27 @@ func main() { debug := flag.Bool("debug", false, "Enable debug logging") // Database connection flags - dbHost := flag.String("db-host", "", "Database host (overrides config)") - dbPort := flag.Int("db-port", 0, "Database port (overrides config)") - dbName := flag.String("db-name", "", "Database name (overrides config)") - dbUser := flag.String("db-user", "", "Database user (overrides config)") - dbPasswordFile := flag.String("db-password-file", "", "Path to file containing the database password") - dbSSLMode := flag.String("db-sslmode", "", "Database SSL mode (overrides config)") + dbHost := flag.String(flagDBHost, "", "Database host (overrides config)") + dbPort := flag.Int(flagDBPort, 0, "Database port (overrides config)") + dbName := flag.String(flagDBName, "", "Database name (overrides config)") + dbUser := flag.String(flagDBUser, "", "Database user (overrides config)") + dbPasswordFile := flag.String(flagDBPasswordFile, "", "Path to file containing the database password") + dbSSLMode := flag.String(flagDBSSLMode, "", "Database SSL mode (overrides config)") flag.Parse() + // Record which flags the operator actually passed, so overrides + // are driven by intent rather than by a value comparison. + overrides := flagOverrides{ + DBHost: *dbHost, + DBPort: *dbPort, + DBName: *dbName, + DBUser: *dbUser, + DBPasswordFile: *dbPasswordFile, + DBSSLMode: *dbSSLMode, + Passed: flagutil.Passed(flag.CommandLine), + } + // Resolve the config path: an explicit flag wins, otherwise the // shared discovery helper picks the highest-priority path that // exists. If neither exists, the resolved path is "" and the @@ -202,7 +232,7 @@ func main() { } // Apply command line overrides - if err := applyFlagOverrides(cfg, *dbHost, *dbPort, *dbName, *dbUser, *dbPasswordFile, *dbSSLMode); err != nil { + if err := applyFlagOverrides(cfg, overrides); err != nil { fmt.Fprintf(os.Stderr, "ERROR: %v\n", err) os.Exit(1) } @@ -262,14 +292,7 @@ func main() { os.Stderr, resolvedConfigPath, explicitConfigPath, - reloadFlagOverrides{ - DBHost: *dbHost, - DBPort: *dbPort, - DBName: *dbName, - DBUser: *dbUser, - DBPasswordFile: *dbPasswordFile, - DBSSLMode: *dbSSLMode, - }, + overrides, ) if err != nil { fmt.Fprintf(os.Stderr, @@ -305,28 +328,41 @@ func main() { // applyFlagOverrides applies CLI flag values to the configuration, allowing // command-line arguments to take precedence over the configuration file. -func applyFlagOverrides(cfg *config.Config, dbHost string, dbPort int, dbName, dbUser, dbPasswordFile, dbSSLMode string) error { - if dbHost != "" { - cfg.Datastore.Host = dbHost +// +// Only the flags recorded in o.Passed are applied, so a value read +// from the configuration file survives unless the operator asked for +// something else, and an explicitly passed flag wins even when its +// value coincides with the flag's registered default. +func applyFlagOverrides(cfg *config.Config, o flagOverrides) error { + if o.Passed.Has(flagDBHost) { + cfg.Datastore.Host = o.DBHost } - if dbPort != 0 { - cfg.Datastore.Port = dbPort + if o.Passed.Has(flagDBPort) { + cfg.Datastore.Port = o.DBPort } - if dbName != "" { - cfg.Datastore.Database = dbName + if o.Passed.Has(flagDBName) { + cfg.Datastore.Database = o.DBName } - if dbUser != "" { - cfg.Datastore.Username = dbUser + if o.Passed.Has(flagDBUser) { + cfg.Datastore.Username = o.DBUser } - if dbPasswordFile != "" { - password, err := fileutil.ReadSecretFile(dbPasswordFile) - if err != nil { - return fmt.Errorf("failed to read password file: %w", err) + // The flag always replaces whatever the configuration file named, + // including with an explicit empty value meaning "no password + // file"; otherwise cfg.LoadPassword would still read the + // configured file later. Only a non-empty path is read here, + // because reading the empty path would fail. + if o.Passed.Has(flagDBPasswordFile) { + cfg.Datastore.PasswordFile = o.DBPasswordFile + if o.DBPasswordFile != "" { + password, err := fileutil.ReadSecretFile(o.DBPasswordFile) + if err != nil { + return fmt.Errorf("failed to read password file: %w", err) + } + cfg.Datastore.Password = password } - cfg.Datastore.Password = password } - if dbSSLMode != "" { - cfg.Datastore.SSLMode = dbSSLMode + if o.Passed.Has(flagDBSSLMode) { + cfg.Datastore.SSLMode = o.DBSSLMode } return nil } diff --git a/alerter/src/cmd/ai-dba-alerter/main_test.go b/alerter/src/cmd/ai-dba-alerter/main_test.go index 5ac66abd..d599fa3b 100644 --- a/alerter/src/cmd/ai-dba-alerter/main_test.go +++ b/alerter/src/cmd/ai-dba-alerter/main_test.go @@ -18,6 +18,7 @@ import ( "github.com/pgedge/ai-workbench/alerter/internal/config" "github.com/pgedge/ai-workbench/pkg/fileutil" + "github.com/pgedge/ai-workbench/pkg/flagutil" ) // minimalValidYAML is a config payload that satisfies @@ -111,7 +112,7 @@ func TestReloadConfigOnSignal_ExplicitMissing(t *testing.T) { &buf, "/definitely/not/a/real/path.yaml", true, // explicit - reloadFlagOverrides{}, + flagOverrides{}, ) if err != nil { t.Fatalf("reloadConfigOnSignal: unexpected error %v", err) @@ -141,7 +142,7 @@ func TestReloadConfigOnSignal_NoCandidateFile(t *testing.T) { &buf, "", // no previously-resolved path either false, - reloadFlagOverrides{}, + flagOverrides{}, ) if err != nil { t.Fatalf("reloadConfigOnSignal: unexpected error %v", err) @@ -168,7 +169,7 @@ func TestReloadConfigOnSignal_HappyPath(t *testing.T) { &buf, cfgPath, true, - reloadFlagOverrides{}, + flagOverrides{}, ) if err != nil { t.Fatalf("reloadConfigOnSignal: %v", err) @@ -185,7 +186,7 @@ func TestReloadConfigOnSignal_HappyPath(t *testing.T) { } // TestReloadConfigOnSignal_FlagOverridesApplied verifies that the -// CLI flag overrides bundled in reloadFlagOverrides are applied +// CLI flag overrides bundled in flagOverrides are applied // to the freshly loaded config so SIGHUP does not silently drop // command-line settings. func TestReloadConfigOnSignal_FlagOverridesApplied(t *testing.T) { @@ -200,10 +201,15 @@ func TestReloadConfigOnSignal_FlagOverridesApplied(t *testing.T) { &buf, cfgPath, true, - reloadFlagOverrides{ + flagOverrides{ DBHost: "override-host", DBPort: 9999, DBSSLMode: "require", + Passed: flagutil.Set{ + flagDBHost: true, + flagDBPort: true, + flagDBSSLMode: true, + }, }, ) if err != nil { @@ -243,7 +249,7 @@ func TestReloadConfigOnSignal_InvalidConfig(t *testing.T) { &buf, cfgPath, true, - reloadFlagOverrides{}, + flagOverrides{}, ) if err == nil { t.Fatal("expected validation error, got nil") @@ -270,7 +276,7 @@ func TestReloadConfigOnSignal_MalformedYAML(t *testing.T) { &buf, cfgPath, true, - reloadFlagOverrides{}, + flagOverrides{}, ) if err == nil { t.Fatal("expected YAML parse error, got nil") @@ -298,7 +304,7 @@ func TestReloadConfigOnSignal_APIKeyWarning(t *testing.T) { &buf, cfgPath, true, - reloadFlagOverrides{}, + flagOverrides{}, ) if err != nil { t.Fatalf("reloadConfigOnSignal: %v", err) @@ -333,7 +339,7 @@ func TestReloadConfigOnSignal_PasswordFileMissing(t *testing.T) { &buf, cfgPath, true, - reloadFlagOverrides{}, + flagOverrides{}, ) if err == nil { t.Fatal("expected password-load error, got nil") @@ -352,7 +358,14 @@ func TestReloadConfigOnSignal_PasswordFileMissing(t *testing.T) { func TestApplyFlagOverrides(t *testing.T) { t.Run("all scalar overrides applied", func(t *testing.T) { cfg := config.NewConfig() - if err := applyFlagOverrides(cfg, "h", 5433, "db", "user", "", "require"); err != nil { + if err := applyFlagOverrides(cfg, flagOverrides{ + DBHost: "h", DBPort: 5433, DBName: "db", DBUser: "user", + DBSSLMode: "require", + Passed: flagutil.Set{ + flagDBHost: true, flagDBPort: true, flagDBName: true, + flagDBUser: true, flagDBSSLMode: true, + }, + }); err != nil { t.Fatalf("unexpected error: %v", err) } if cfg.Datastore.Host != "h" || cfg.Datastore.Port != 5433 || @@ -365,7 +378,7 @@ func TestApplyFlagOverrides(t *testing.T) { t.Run("no overrides leaves config untouched", func(t *testing.T) { cfg := config.NewConfig() host := cfg.Datastore.Host - if err := applyFlagOverrides(cfg, "", 0, "", "", "", ""); err != nil { + if err := applyFlagOverrides(cfg, flagOverrides{}); err != nil { t.Fatalf("unexpected error: %v", err) } if cfg.Datastore.Host != host { @@ -379,7 +392,10 @@ func TestApplyFlagOverrides(t *testing.T) { t.Fatalf("write: %v", err) } cfg := config.NewConfig() - if err := applyFlagOverrides(cfg, "", 0, "", "", pwFile, ""); err != nil { + if err := applyFlagOverrides(cfg, flagOverrides{ + DBPasswordFile: pwFile, + Passed: flagutil.Set{flagDBPasswordFile: true}, + }); err != nil { t.Fatalf("unexpected error: %v", err) } if cfg.Datastore.Password != "flag-password" { @@ -389,7 +405,10 @@ func TestApplyFlagOverrides(t *testing.T) { t.Run("missing password file is an error", func(t *testing.T) { cfg := config.NewConfig() - if err := applyFlagOverrides(cfg, "", 0, "", "", "/nonexistent/pw", ""); err == nil { + if err := applyFlagOverrides(cfg, flagOverrides{ + DBPasswordFile: "/nonexistent/pw", + Passed: flagutil.Set{flagDBPasswordFile: true}, + }); err == nil { t.Error("expected error for missing password file") } }) @@ -400,7 +419,10 @@ func TestApplyFlagOverrides(t *testing.T) { t.Fatalf("write: %v", err) } cfg := config.NewConfig() - if err := applyFlagOverrides(cfg, "", 0, "", "", pwFile, ""); err == nil { + if err := applyFlagOverrides(cfg, flagOverrides{ + DBPasswordFile: pwFile, + Passed: flagutil.Set{flagDBPasswordFile: true}, + }); err == nil { t.Error("expected error for empty password file") } }) diff --git a/collector/src/config.go b/collector/src/config.go index 3f1f174f..7c917d8b 100644 --- a/collector/src/config.go +++ b/collector/src/config.go @@ -16,6 +16,7 @@ import ( "github.com/pgedge/ai-workbench/pkg/datastoreconfig" "github.com/pgedge/ai-workbench/pkg/fileutil" + "github.com/pgedge/ai-workbench/pkg/flagutil" "gopkg.in/yaml.v3" ) @@ -89,36 +90,43 @@ func (c *Config) LoadFromFile(filename string) error { return nil } -// ApplyFlags applies command line flags to override config values -func (c *Config) ApplyFlags() { - if *pgHost != "" { +// ApplyFlags applies command line flags to override config values. +// +// The passed set names the flags the operator actually supplied on +// the command line, as reported by flagutil.Passed. Only those flags +// override the configuration file, so a file value that happens to +// equal a flag's registered default survives, and a flag explicitly +// passed with its default value still wins. Deciding on the value +// alone cannot tell those two cases apart. +func (c *Config) ApplyFlags(passed flagutil.Set) { + if passed.Has(flagPGHost) { c.Datastore.Host = *pgHost } - if *pgHostAddr != "" { + if passed.Has(flagPGHostAddr) { c.Datastore.HostAddr = *pgHostAddr } - if *pgDatabase != "" { + if passed.Has(flagPGDatabase) { c.Datastore.Database = *pgDatabase } - if *pgUsername != "" { + if passed.Has(flagPGUsername) { c.Datastore.Username = *pgUsername } - if *pgPasswordFile != "" { + if passed.Has(flagPGPasswordFile) { c.Datastore.PasswordFile = *pgPasswordFile } - if *pgPort != 5432 { + if passed.Has(flagPGPort) { c.Datastore.Port = *pgPort } - if *pgSSLMode != "prefer" { + if passed.Has(flagPGSSLMode) { c.Datastore.SSLMode = *pgSSLMode } - if *pgSSLCert != "" { + if passed.Has(flagPGSSLCert) { c.Datastore.SSLCert = *pgSSLCert } - if *pgSSLKey != "" { + if passed.Has(flagPGSSLKey) { c.Datastore.SSLKey = *pgSSLKey } - if *pgSSLRootCert != "" { + if passed.Has(flagPGSSLRootCert) { c.Datastore.SSLRootCert = *pgSSLRootCert } } diff --git a/collector/src/config_test.go b/collector/src/config_test.go index af22848b..3d6bb2de 100644 --- a/collector/src/config_test.go +++ b/collector/src/config_test.go @@ -16,6 +16,7 @@ import ( "github.com/pgedge/ai-workbench/pkg/datastoreconfig" "github.com/pgedge/ai-workbench/pkg/fileutil" + "github.com/pgedge/ai-workbench/pkg/flagutil" ) func TestNewConfig(t *testing.T) { @@ -469,6 +470,24 @@ func TestConfigLoadFromFile_InvalidYAML(t *testing.T) { } } +// allDatastoreFlagsPassed returns a flagutil.Set naming every +// datastore connection flag, as if the operator had supplied all of +// them on the command line. +func allDatastoreFlagsPassed() flagutil.Set { + return flagutil.Set{ + flagPGHost: true, + flagPGHostAddr: true, + flagPGDatabase: true, + flagPGUsername: true, + flagPGPasswordFile: true, + flagPGPort: true, + flagPGSSLMode: true, + flagPGSSLCert: true, + flagPGSSLKey: true, + flagPGSSLRootCert: true, + } +} + func TestConfigApplyFlags(t *testing.T) { // Save and restore the package-level flag pointers so this test is // isolated from other tests in the file. @@ -502,8 +521,9 @@ func TestConfigApplyFlags(t *testing.T) { *pgSSLKey = "/flag/key" *pgSSLRootCert = "/flag/root" + // Every flag is marked as passed, so every override applies. config := NewConfig() - config.ApplyFlags() + config.ApplyFlags(allDatastoreFlagsPassed()) if config.Datastore.Host != "flag-host" { t.Errorf("Host: got %q, want flag-host", config.Datastore.Host) @@ -537,7 +557,7 @@ func TestConfigApplyFlags(t *testing.T) { } } -func TestConfigApplyFlags_EmptyFlagsPreserveDefaults(t *testing.T) { +func TestConfigApplyFlags_UnpassedFlagsPreserveConfig(t *testing.T) { // Save and restore flag pointers. origHost, origHostAddr := *pgHost, *pgHostAddr origDB, origUser := *pgDatabase, *pgUsername @@ -557,18 +577,20 @@ func TestConfigApplyFlags_EmptyFlagsPreserveDefaults(t *testing.T) { *pgSSLRootCert = origSSLRootCert }) - // Set all flags to their default values: ApplyFlags should not - // overwrite any config field in this case. - *pgHost = "" - *pgHostAddr = "" - *pgDatabase = "" - *pgUsername = "" - *pgPasswordFile = "" + // Give every flag a value that differs from the configuration + // below, but mark none of them as passed. ApplyFlags must ignore + // the values entirely, because nothing was supplied on the + // command line. + *pgHost = "flag-host" + *pgHostAddr = "10.0.0.5" + *pgDatabase = "flag-db" + *pgUsername = "flag-user" + *pgPasswordFile = "/flag/password" *pgPort = 5432 *pgSSLMode = "prefer" - *pgSSLCert = "" - *pgSSLKey = "" - *pgSSLRootCert = "" + *pgSSLCert = "/flag/cert" + *pgSSLKey = "/flag/key" + *pgSSLRootCert = "/flag/root" config := NewConfig() // Pre-set all fields so we can detect any unwanted overwrite. @@ -583,7 +605,7 @@ func TestConfigApplyFlags_EmptyFlagsPreserveDefaults(t *testing.T) { config.Datastore.SSLKey = "/preserved/key" config.Datastore.SSLRootCert = "/preserved/root" - config.ApplyFlags() + config.ApplyFlags(nil) if config.Datastore.Host != "preserved" { t.Errorf("Host should not have changed, got %q", config.Datastore.Host) @@ -617,6 +639,102 @@ func TestConfigApplyFlags_EmptyFlagsPreserveDefaults(t *testing.T) { } } +// TestConfigApplyFlags_ExplicitDefaultValueWins covers the case the +// old value-comparison logic could not express: the operator passes a +// flag whose value happens to equal the flag's registered default, so +// the flag must still override the configuration file. +func TestConfigApplyFlags_ExplicitDefaultValueWins(t *testing.T) { + origPort, origSSLMode := *pgPort, *pgSSLMode + t.Cleanup(func() { + *pgPort = origPort + *pgSSLMode = origSSLMode + }) + + // Both values are the flags' registered defaults. + *pgPort = 5432 + *pgSSLMode = "prefer" + + config := NewConfig() + // Values as if they had come from a configuration file. + config.Datastore.Port = 6000 + config.Datastore.SSLMode = "require" + + config.ApplyFlags(flagutil.Set{flagPGPort: true, flagPGSSLMode: true}) + + if config.Datastore.Port != 5432 { + t.Errorf("Port: got %d, want 5432 from the explicit flag", config.Datastore.Port) + } + if config.Datastore.SSLMode != "prefer" { + t.Errorf("SSLMode: got %q, want prefer from the explicit flag", config.Datastore.SSLMode) + } +} + +// TestConfigApplyFlags_ConfigMatchingFlagDefaultSurvives covers the +// mirror image: a configuration file value that coincides with a +// flag's registered default must survive when the flag is not passed. +func TestConfigApplyFlags_ConfigMatchingFlagDefaultSurvives(t *testing.T) { + origPort, origSSLMode := *pgPort, *pgSSLMode + t.Cleanup(func() { + *pgPort = origPort + *pgSSLMode = origSSLMode + }) + + *pgPort = 5432 + *pgSSLMode = "prefer" + + config := NewConfig() + config.Datastore.Port = 5432 + config.Datastore.SSLMode = "prefer" + + config.ApplyFlags(nil) + + if config.Datastore.Port != 5432 { + t.Errorf("Port: got %d, want the config value 5432", config.Datastore.Port) + } + if config.Datastore.SSLMode != "prefer" { + t.Errorf("SSLMode: got %q, want the config value prefer", config.Datastore.SSLMode) + } +} + +// TestLoadConfigurationFlagPrecedence drives the whole load path: a +// configuration file sets a non-default port, and an explicitly +// passed -pg-port carrying the flag's own default value must still +// win, which is precisely the case the old logic got wrong. +func TestLoadConfigurationFlagPrecedence(t *testing.T) { + tmpDir := t.TempDir() + configPath := filepath.Join(tmpDir, "collector.yaml") + secretPath := filepath.Join(tmpDir, "collector.secret") + + yaml := "datastore:\n host: config-host\n port: 6000\n sslmode: require\n" + + "secret_file: " + secretPath + "\n" + if err := os.WriteFile(configPath, []byte(yaml), 0600); err != nil { + t.Fatalf("write config: %v", err) + } + if err := os.WriteFile(secretPath, []byte("test-secret\n"), 0600); err != nil { + t.Fatalf("write secret: %v", err) + } + + defer saveAndClearFlags(t)() + *configFile = configPath + *pgPort = 5432 + *pgSSLMode = "prefer" + + // Only -pg-port was passed; -pg-sslmode was not. + cfg, err := loadConfiguration(flagutil.Set{flagPGPort: true}) + if err != nil { + t.Fatalf("loadConfiguration: %v", err) + } + if cfg.Datastore.Port != 5432 { + t.Errorf("Port: got %d, want 5432 from the explicit flag", cfg.Datastore.Port) + } + if cfg.Datastore.SSLMode != "require" { + t.Errorf("SSLMode: got %q, want require from the config file", cfg.Datastore.SSLMode) + } + if cfg.Datastore.Host != "config-host" { + t.Errorf("Host: got %q, want config-host from the config file", cfg.Datastore.Host) + } +} + func TestConfigLoadPassword_AlreadySet(t *testing.T) { config := NewConfig() config.Datastore.Password = "already-set" diff --git a/collector/src/main.go b/collector/src/main.go index cb67c30e..6bf53081 100644 --- a/collector/src/main.go +++ b/collector/src/main.go @@ -12,6 +12,7 @@ package main import ( "github.com/pgedge/ai-workbench/collector/src/database" "github.com/pgedge/ai-workbench/collector/src/scheduler" + "github.com/pgedge/ai-workbench/pkg/flagutil" "github.com/pgedge/ai-workbench/pkg/logger" "context" @@ -40,16 +41,33 @@ var ( "Print the latest datastore schema version this collector knows about and exit") // Datastore connection flags - pgHost = flag.String("pg-host", "", "PostgreSQL server hostname or IP address") - pgHostAddr = flag.String("pg-hostaddr", "", "PostgreSQL server IP address") - pgDatabase = flag.String("pg-database", "", "PostgreSQL database name") - pgUsername = flag.String("pg-username", "", "PostgreSQL username") - pgPasswordFile = flag.String("pg-password-file", "", "Path to file containing PostgreSQL password") - pgPort = flag.Int("pg-port", 5432, "PostgreSQL server port") - pgSSLMode = flag.String("pg-sslmode", "prefer", "PostgreSQL SSL mode") - pgSSLCert = flag.String("pg-sslcert", "", "Path to PostgreSQL client SSL certificate") - pgSSLKey = flag.String("pg-sslkey", "", "Path to PostgreSQL client SSL key") - pgSSLRootCert = flag.String("pg-sslrootcert", "", "Path to PostgreSQL root SSL certificate") + pgHost = flag.String(flagPGHost, "", "PostgreSQL server hostname or IP address") + pgHostAddr = flag.String(flagPGHostAddr, "", "PostgreSQL server IP address") + pgDatabase = flag.String(flagPGDatabase, "", "PostgreSQL database name") + pgUsername = flag.String(flagPGUsername, "", "PostgreSQL username") + pgPasswordFile = flag.String(flagPGPasswordFile, "", "Path to file containing PostgreSQL password") + pgPort = flag.Int(flagPGPort, 5432, "PostgreSQL server port") + pgSSLMode = flag.String(flagPGSSLMode, "prefer", "PostgreSQL SSL mode") + pgSSLCert = flag.String(flagPGSSLCert, "", "Path to PostgreSQL client SSL certificate") + pgSSLKey = flag.String(flagPGSSLKey, "", "Path to PostgreSQL client SSL key") + pgSSLRootCert = flag.String(flagPGSSLRootCert, "", "Path to PostgreSQL root SSL certificate") +) + +// Names of the datastore connection flags. They are named constants +// so the registration above and the override logic in ApplyFlags +// cannot drift apart; ApplyFlags looks a flag up by name to decide +// whether the operator actually passed it. +const ( + flagPGHost = "pg-host" + flagPGHostAddr = "pg-hostaddr" + flagPGDatabase = "pg-database" + flagPGUsername = "pg-username" + flagPGPasswordFile = "pg-password-file" + flagPGPort = "pg-port" + flagPGSSLMode = "pg-sslmode" + flagPGSSLCert = "pg-sslcert" + flagPGSSLKey = "pg-sslkey" + flagPGSSLRootCert = "pg-sslrootcert" ) func main() { @@ -76,7 +94,7 @@ func main() { logger.Startupf("pgEdge AI DBA Workbench Collector v%s starting...", Version) // Load configuration - config, err := loadConfiguration() + config, err := loadConfiguration(flagutil.Passed(flag.CommandLine)) if err != nil { logger.Fatalf("Failed to load configuration: %v", err) } @@ -162,12 +180,16 @@ func maybePrintSchemaVersion(w io.Writer, enabled bool) (bool, error) { // loadConfiguration loads configuration from file, environment, and command line. // Priority (highest to lowest): CLI flags > environment variables > config file > defaults. // +// The passed set names the flags the operator supplied on the command +// line; it is threaded in from main rather than read from the global +// flag set here so tests can drive both branches of every override. +// // When --config is not given, the function consults the shared // helper which searches the per-user config directory first and // /etc/pgedge second. When neither exists the function logs an // informational message and proceeds with compiled-in defaults // rather than failing. -func loadConfiguration() (*Config, error) { +func loadConfiguration(passed flagutil.Set) (*Config, error) { config := NewConfig() // Determine config file path. The empty string from the helper @@ -213,7 +235,7 @@ func loadConfiguration() (*Config, error) { } // Override with command line flags (highest priority) - config.ApplyFlags() + config.ApplyFlags(passed) // Load password from file if specified if err := config.LoadPassword(); err != nil { diff --git a/collector/src/main_test.go b/collector/src/main_test.go index faf09290..e4dcdb16 100644 --- a/collector/src/main_test.go +++ b/collector/src/main_test.go @@ -22,6 +22,7 @@ import ( "github.com/pgedge/ai-workbench/collector/src/database" "github.com/pgedge/ai-workbench/pkg/fileutil" + "github.com/pgedge/ai-workbench/pkg/flagutil" ) // saveAndClearFlags resets the package-level flag pointers and returns a @@ -68,7 +69,7 @@ func TestLoadConfiguration_ExplicitConfigMissing(t *testing.T) { *configFile = "/nonexistent/path/to/config.yaml" - cfg, err := loadConfiguration() + cfg, err := loadConfiguration(nil) if err == nil { t.Fatal("expected error when explicit config file does not exist") } @@ -87,7 +88,7 @@ func TestLoadConfiguration_ExplicitConfigMalformed(t *testing.T) { } *configFile = configPath - _, err := loadConfiguration() + _, err := loadConfiguration(nil) if err == nil { t.Fatal("expected error for malformed YAML") } @@ -105,7 +106,9 @@ func TestLoadConfiguration_PasswordFileMissing(t *testing.T) { *configFile = configPath *pgPasswordFile = "/definitely/missing/password-file" - _, err := loadConfiguration() + // -pg-password-file is marked as passed, so the override applies + // and loadConfiguration then fails to read the named file. + _, err := loadConfiguration(flagutil.Set{flagPGPasswordFile: true}) if err == nil { t.Fatal("expected error because password file is missing") } @@ -129,14 +132,14 @@ func TestLoadConfiguration_SecretFileMissing(t *testing.T) { } *configFile = configPath - _, err := loadConfiguration() + _, err := loadConfiguration(nil) if err == nil { t.Fatal("expected error because server secret file is missing") } } // TestLoadConfiguration_Success exercises the full happy path of -// loadConfiguration(): it writes a config file, a password file, and a +// loadConfiguration: it writes a config file, a password file, and a // secret file all within the temp directory and explicitly references // them in the config YAML. func TestLoadConfiguration_Success(t *testing.T) { @@ -166,7 +169,7 @@ func TestLoadConfiguration_Success(t *testing.T) { *configFile = configPath - cfg, err := loadConfiguration() + cfg, err := loadConfiguration(nil) if err != nil { t.Fatalf("loadConfiguration error: %v", err) } @@ -198,7 +201,7 @@ func TestLoadConfiguration_NoConfigFileUsesDefaults(t *testing.T) { // loadConfiguration must surface an error from LoadSecret since // there is no secret file available in either default location. - _, err := loadConfiguration() + _, err := loadConfiguration(nil) if err == nil { t.Fatal("expected error because no secret file exists on default paths") } @@ -224,7 +227,7 @@ func TestLoadConfiguration_ExplicitConfigPermissionError(t *testing.T) { } *configFile = cfgPath - _, err := loadConfiguration() + _, err := loadConfiguration(nil) if err == nil { t.Fatal("expected error when explicit config path is unreadable") } @@ -264,7 +267,7 @@ func TestLoadConfiguration_AutoDiscoveredUnreadable(t *testing.T) { t.Fatalf("Mkdir: %v", err) } - _, err = loadConfiguration() + _, err = loadConfiguration(nil) if err == nil { t.Fatal("expected error from non-IsNotExist auto-discovery branch") } @@ -309,7 +312,7 @@ func TestLoadConfiguration_DefaultConfigFromUserDir(t *testing.T) { t.Fatalf("write cfg: %v", err) } - cfg, err := loadConfiguration() + cfg, err := loadConfiguration(nil) if err != nil { t.Fatalf("loadConfiguration: %v", err) } diff --git a/docs/changelog.md b/docs/changelog.md index 1d847e9f..46e19830 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -61,6 +61,18 @@ project adheres to ### Fixed +- Fix the collector and the alerter overriding a configuration file + value whenever that value happened to match a command-line flag's + built-in default, and ignoring a flag that an operator passed + explicitly with its default value. Both services decided whether a + flag had been supplied by comparing the flag's value against its + own default, which cannot tell the two cases apart; for example, + a collector configuration file setting `datastore.port: 6000` + ignored `-pg-port 5432` on the command line. Both services now + detect the flags that were actually present on the command line, + so the documented precedence of defaults, then configuration file, + then flags holds in every case. (#389) + - Fix every chat request that included a tool list failing with `anthropic (400): tools.0.custom.input_schema: Input does not match the expected shape`, which broke Ask Ellie and the Server, diff --git a/docs/getting-started/configuration/alerter.md b/docs/getting-started/configuration/alerter.md index 5fb1212d..564c8e7d 100644 --- a/docs/getting-started/configuration/alerter.md +++ b/docs/getting-started/configuration/alerter.md @@ -13,6 +13,12 @@ following order; later sources override earlier ones: 2. Configuration file settings (YAML format). 3. Command-line flag overrides. +The alerter overrides a configuration file setting only when you +actually pass the corresponding flag; a flag you omit never +overrides the file, even though the flag has a built-in default. A +flag you do pass always wins, including when the value you give it +happens to equal that flag's default. + ## Configuration File The alerter searches for its configuration file in the diff --git a/docs/getting-started/configuration/collector.md b/docs/getting-started/configuration/collector.md index 9f1891fc..399264f0 100644 --- a/docs/getting-started/configuration/collector.md +++ b/docs/getting-started/configuration/collector.md @@ -8,6 +8,12 @@ order; later sources override earlier ones: 2. Configuration file. 3. Command-line flags. +The collector overrides a configuration file setting only when you +actually pass the corresponding flag; a flag you omit never +overrides the file, even though the flag has a built-in default. A +flag you do pass always wins, including when the value you give it +happens to equal that flag's default, such as `-pg-port 5432`. + ## File Location The collector searches for its configuration file in the following diff --git a/pkg/flagutil/flagutil.go b/pkg/flagutil/flagutil.go new file mode 100644 index 00000000..d2cafff9 --- /dev/null +++ b/pkg/flagutil/flagutil.go @@ -0,0 +1,47 @@ +/*------------------------------------------------------------------------- + * + * pgEdge AI DBA Workbench + * + * Copyright (c) 2025 - 2026, pgEdge, Inc. + * This software is released under The PostgreSQL License + * + *------------------------------------------------------------------------- + */ + +// Package flagutil provides helpers for reasoning about command line +// flags. Configuration in the Workbench is layered (built-in defaults, +// then the configuration file, then command line flags), which means a +// caller must be able to tell "the operator passed this flag" from "the +// flag is sitting at its registered default". Comparing a flag's value +// against its default cannot make that distinction, because an operator +// may legitimately pass a value that happens to equal the default. +package flagutil + +import "flag" + +// Set records the names of the flags that were explicitly present on +// the command line. The zero value is usable: a nil Set simply reports +// that no flag was passed. +type Set map[string]bool + +// Passed returns the Set of flags that fs saw on the command line. +// Flags left at their registered defaults are absent from the result, +// so callers can apply an override only when the operator asked for +// it. A nil FlagSet yields an empty Set rather than a panic, which +// keeps callers free of nil checks. +func Passed(fs *flag.FlagSet) Set { + passed := make(Set) + if fs == nil { + return passed + } + fs.Visit(func(f *flag.Flag) { + passed[f.Name] = true + }) + return passed +} + +// Has reports whether the named flag was explicitly passed on the +// command line. +func (s Set) Has(name string) bool { + return s[name] +} diff --git a/pkg/flagutil/flagutil_test.go b/pkg/flagutil/flagutil_test.go new file mode 100644 index 00000000..d282149f --- /dev/null +++ b/pkg/flagutil/flagutil_test.go @@ -0,0 +1,95 @@ +/*------------------------------------------------------------------------- + * + * pgEdge AI DBA Workbench + * + * Copyright (c) 2025 - 2026, pgEdge, Inc. + * This software is released under The PostgreSQL License + * + *------------------------------------------------------------------------- + */ +package flagutil + +import ( + "flag" + "io" + "testing" +) + +// newTestFlagSet builds a flag set with a string and an int flag whose +// defaults mirror the collector's, so tests can pass a value that +// coincides with a default. +func newTestFlagSet() (*flag.FlagSet, *string, *int) { + fs := flag.NewFlagSet("test", flag.ContinueOnError) + fs.SetOutput(io.Discard) + sslMode := fs.String("pg-sslmode", "prefer", "SSL mode") + port := fs.Int("pg-port", 5432, "port") + return fs, sslMode, port +} + +func TestPassed_NoFlags(t *testing.T) { + fs, _, _ := newTestFlagSet() + if err := fs.Parse(nil); err != nil { + t.Fatalf("Parse: %v", err) + } + + passed := Passed(fs) + if len(passed) != 0 { + t.Errorf("Passed() = %v, want an empty set", passed) + } + if passed.Has("pg-port") { + t.Error("Has(pg-port) = true, want false when nothing was passed") + } +} + +// TestPassed_ExplicitDefaultValue is the case that motivates the +// package: a flag passed with the value that is also its registered +// default must still be reported as passed. +func TestPassed_ExplicitDefaultValue(t *testing.T) { + fs, sslMode, port := newTestFlagSet() + if err := fs.Parse([]string{"-pg-port", "5432", "-pg-sslmode", "prefer"}); err != nil { + t.Fatalf("Parse: %v", err) + } + + passed := Passed(fs) + if !passed.Has("pg-port") { + t.Error("Has(pg-port) = false, want true for an explicitly passed flag") + } + if !passed.Has("pg-sslmode") { + t.Error("Has(pg-sslmode) = false, want true for an explicitly passed flag") + } + if *port != 5432 || *sslMode != "prefer" { + t.Errorf("parsed values changed unexpectedly: port=%d sslmode=%q", *port, *sslMode) + } +} + +func TestPassed_SubsetOfFlags(t *testing.T) { + fs, _, _ := newTestFlagSet() + if err := fs.Parse([]string{"-pg-port", "6000"}); err != nil { + t.Fatalf("Parse: %v", err) + } + + passed := Passed(fs) + if !passed.Has("pg-port") { + t.Error("Has(pg-port) = false, want true") + } + if passed.Has("pg-sslmode") { + t.Error("Has(pg-sslmode) = true, want false for an unpassed flag") + } +} + +func TestPassed_NilFlagSet(t *testing.T) { + passed := Passed(nil) + if passed == nil { + t.Fatal("Passed(nil) = nil, want an empty set") + } + if passed.Has("anything") { + t.Error("Has() on an empty set = true, want false") + } +} + +func TestSet_HasOnNilSet(t *testing.T) { + var s Set + if s.Has("pg-port") { + t.Error("Has() on a nil Set = true, want false") + } +}