From 169c6c6b641bf6e3fd11d6b4d54ed9c5607b3962 Mon Sep 17 00:00:00 2001 From: Craig Williams Date: Tue, 13 Oct 2026 13:19:01 +0100 Subject: [PATCH] `config` updated to improve setting values, writing changes and test coverage --- internal/config/README.md | 6 +- internal/config/conf_sections_test.go | 67 ++++++++++++ internal/config/config.go | 37 ++++--- internal/config/modify.go | 112 ++++++++++++-------- internal/config/modify_test.go | 141 ++++++++++++++++++++++++++ internal/config/print.go | 11 +- internal/config/print_test.go | 129 +++++++++++++++++++++++ internal/config/section_test.go | 9 ++ 8 files changed, 453 insertions(+), 59 deletions(-) create mode 100644 internal/config/print_test.go diff --git a/internal/config/README.md b/internal/config/README.md index afc268e..58b4bf0 100644 --- a/internal/config/README.md +++ b/internal/config/README.md @@ -116,10 +116,12 @@ Nothing in `section.go` needs changing. - `conf.Map()` (nested) and `conf.Flatten()` (dotted keys): the whole configuration as a map. - `conf.String()`, `conf.JSON()`, `conf.YAML()` and the matching `Print*` - methods: the configuration as text. + methods: the configuration as text, with keys sorted. - `StoreValue(&conf, key, value)` / `PurgeValue(&conf, key)`: set or remove a key in the config file Viper loaded. Used by `config set` and - `config set --unset`. `StoreValue` validates the value before writing it. + `config set --unset`. `StoreValue` validates the value before writing it, + and stores it as the field's own type. Only the file's own settings are + written back; defaults and environment variables never are. - `LoadDefaultConfig`, `LoadToViper`, `LoadFromViper`: move values between the struct defaults, Viper and a `JobSearchConfig`. diff --git a/internal/config/conf_sections_test.go b/internal/config/conf_sections_test.go index 8e23fc3..084fa7a 100644 --- a/internal/config/conf_sections_test.go +++ b/internal/config/conf_sections_test.go @@ -455,3 +455,70 @@ func TestLoadFromViperInvalidValues(t *testing.T) { } } } + +func TestDatabaseSection(t *testing.T) { + c := JobSearchConfigDatabase{File: "./jobsearch.db"} + + if got, want := c.Keys(), []string{"file"}; !reflect.DeepEqual(got, want) { + t.Errorf("Keys = %v; want %v", got, want) + } + if got, want := c.Map(), map[string]any{"file": "./jobsearch.db"}; !reflect.DeepEqual(got, want) { + t.Errorf("Map = %v; want %v", got, want) + } + want := map[string]any{"database.file": "./jobsearch.db"} + if got := c.Flatten(); !reflect.DeepEqual(got, want) { + t.Errorf("Flatten = %v; want %v", got, want) + } + if k, err := c.TypeOf("file"); err != nil || k != reflect.String { + t.Errorf("TypeOf(file) = %v, %v", k, err) + } + if v, err := c.ValueOf("file"); err != nil || v != "./jobsearch.db" { + t.Errorf("ValueOf(file) = %v, %v", v, err) + } + if _, err := c.TypeOf("nope"); err == nil { + t.Error("TypeOf(nope) succeeded") + } + if _, err := c.ValueOf("nope"); err == nil { + t.Error("ValueOf(nope) succeeded") + } + if err := c.SetValue("file", "/var/lib/js.db"); err != nil || c.File != "/var/lib/js.db" { + t.Errorf("SetValue(file) = %v, File = %q", err, c.File) + } + if err := c.SetValue("file", 42); err == nil { + t.Error("SetValue(file, 42) succeeded") + } + + // The root configuration reaches it as database.file. + conf := defaultConfig(t) + if err := conf.SetValue("database.file", "./other.db"); err != nil || + conf.Database.File != "./other.db" { + t.Errorf("SetValue(database.file) = %v, File = %q", err, conf.Database.File) + } +} + +func TestViperValue(t *testing.T) { + resetViper(t) + viper.Set("s", "text") + viper.Set("i", "42") // strings are converted + viper.Set("b", "true") + + for _, tt := range []struct { + key string + kind reflect.Kind + want any + }{ + {"s", reflect.String, "text"}, + {"i", reflect.Int, 42}, + {"b", reflect.Bool, true}, + } { + got, ok := viperValue(tt.key, tt.kind) + if !ok || got != tt.want { + t.Errorf("viperValue(%q, %v) = %#v, %v; want %#v", tt.key, tt.kind, got, ok, tt.want) + } + } + for _, kind := range []reflect.Kind{reflect.Float64, reflect.Int8, reflect.Slice} { + if got, ok := viperValue("i", kind); ok { + t.Errorf("viperValue(i, %v) = %#v, true; want unhandled", kind, got) + } + } +} diff --git a/internal/config/config.go b/internal/config/config.go index 56e18b0..4a1fa47 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -4,7 +4,9 @@ import ( "encoding/json" "fmt" "log/slog" + "maps" "reflect" + "slices" "strings" "github.com/spf13/viper" @@ -65,23 +67,31 @@ func LoadFromViper(conf *JobSearchConfig) { if keyType == reflect.Invalid { continue } - var err error - switch keyType { - case reflect.String: - err = conf.SetValue(key, viper.GetString(key)) - case reflect.Int: - err = conf.SetValue(key, viper.GetInt(key)) - case reflect.Bool: - err = conf.SetValue(key, viper.GetBool(key)) - default: + value, ok := viperValue(key, keyType) + if !ok { slog.Error(fmt.Sprintf("unknown key type: %s", keyType)) + continue } - if err != nil { + if err := conf.SetValue(key, value); err != nil { slog.Warn("ignoring invalid config value", "key", key, "err", err) } } } +// viperValue reads key from Viper as a value of kind. ok is false for a kind +// LoadFromViper doesn't handle. +func viperValue(key string, kind reflect.Kind) (value any, ok bool) { + switch kind { + case reflect.String: + return viper.GetString(key), true + case reflect.Int: + return viper.GetInt(key), true + case reflect.Bool: + return viper.GetBool(key), true + } + return nil, false +} + // LoadToViper sets every key of conf as a Viper default. func LoadToViper(conf JobSearchConfig) { flatMap := conf.Flatten() @@ -91,12 +101,13 @@ func LoadToViper(conf JobSearchConfig) { } // String formats the configuration one "key = value " line per key, with -// dotted keys. +// dotted keys, sorted by key. func (conf JobSearchConfig) String() string { var sb strings.Builder - for k, v := range conf.Flatten() { + flat := conf.Flatten() + for _, k := range slices.Sorted(maps.Keys(flat)) { typ, _ := conf.TypeOf(k) - fmt.Fprint(&sb, k, " = ", v, " <", typ, ">\n") + fmt.Fprint(&sb, k, " = ", flat[k], " <", typ, ">\n") } return sb.String() } diff --git a/internal/config/modify.go b/internal/config/modify.go index 7ed9499..3296f05 100644 --- a/internal/config/modify.go +++ b/internal/config/modify.go @@ -10,74 +10,106 @@ import ( ) // StoreValue parses value as the type of key, checks conf accepts it, sets it -// in conf and Viper and writes the config file Viper is using. It returns the -// parsed value and its kind. +// in conf and Viper and writes it to the config file Viper is using. Only the +// file's own settings are written back, not defaults or environment +// variables. It returns the stored value (as ValueOf gives it) and its kind. func StoreValue( conf *JobSearchConfig, key string, value string, ) (any, reflect.Kind, error) { - var confValue any = nil - var confType reflect.Kind - var err error + return storeValue(conf, key, value) +} - confType, err = conf.TypeOf(key) +// storeValue is StoreValue for any section, so key types that +// JobSearchConfig doesn't have yet can be tested. +func storeValue(s Section, key string, value string) (any, reflect.Kind, error) { + confType, err := s.TypeOf(key) if err != nil { return nil, confType, err } - switch confType { - case reflect.Bool: - confValue, err = strconv.ParseBool(value) - case reflect.String: - confValue = value - case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64: - confValue, err = strconv.ParseInt(value, 10, 64) - case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64: - confValue, err = strconv.ParseUint(value, 10, 64) - case reflect.Float32, reflect.Float64: - confValue, err = strconv.ParseFloat(value, 64) - default: - err = errors.ErrUnsupported - } - if err != nil { + // Check the value is of the right kind first, for a clearer error than + // SetValue gives. + if _, err := parseValue(confType, value); err != nil { return nil, confType, fmt.Errorf( "config value type not accepted, expected <%s>", confType, ) } - // Reject values the configuration itself would not accept, e.g. an - // unknown log level or an unparseable duration. - if err := conf.SetValue(key, confValue); err != nil { + // SetValue parses the string into the field's own type (e.g. int8, + // float32 or time.Duration) and rejects values the configuration itself + // would not accept, e.g. an unknown log level or an unparseable duration. + if err := s.SetValue(key, value); err != nil { + return nil, confType, err + } + confValue, err := s.ValueOf(key) + if err != nil { return nil, confType, err } viper.Set(key, confValue) - if err := viper.WriteConfig(); err != nil { + file, err := readUsedConfigFile() + if err != nil { + return nil, confType, err + } + file.Set(key, confValue) + if err := file.WriteConfig(); err != nil { return nil, confType, err } return confValue, confType, nil } -// PurgeValue rewrites the config file Viper is using without key. +// parseValue parses value as a value of kind, returning +// errors.ErrUnsupported for a kind it can't parse. +func parseValue(kind reflect.Kind, value string) (any, error) { + switch kind { + case reflect.Bool: + return strconv.ParseBool(value) + case reflect.String: + return value, nil + case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64: + return strconv.ParseInt(value, 10, 64) + case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64: + return strconv.ParseUint(value, 10, 64) + case reflect.Float32, reflect.Float64: + return strconv.ParseFloat(value, 64) + } + return nil, errors.ErrUnsupported +} + +// PurgeValue rewrites the config file Viper is using without key. The +// file's other settings are kept as they are; defaults and environment +// variables are not written to it. func PurgeValue(conf *JobSearchConfig, key string) error { if _, err := conf.TypeOf(key); err != nil { return err } - v := viper.New() - v.SetConfigFile(viper.ConfigFileUsed()) - - keys := viper.AllKeys() - for i := range keys { - k := keys[i] - if k != key { - v.Set(k, viper.Get(k)) - } - } - - if err := v.WriteConfig(); err != nil { + file, err := readUsedConfigFile() + if err != nil { return err } - - return nil + out := viper.New() + out.SetConfigFile(file.ConfigFileUsed()) + for _, k := range file.AllKeys() { + if k != key { + out.Set(k, file.Get(k)) + } + } + return out.WriteConfig() +} + +// readUsedConfigFile reads the config file Viper is using into a new Viper, so +// it holds only the file's settings. +func readUsedConfigFile() (*viper.Viper, error) { + path := viper.ConfigFileUsed() + if path == "" { + return nil, errors.New("no configuration file in use") + } + file := viper.New() + file.SetConfigFile(path) + if err := file.ReadInConfig(); err != nil { + return nil, err + } + return file, nil } diff --git a/internal/config/modify_test.go b/internal/config/modify_test.go index d54b168..18610a3 100644 --- a/internal/config/modify_test.go +++ b/internal/config/modify_test.go @@ -1,6 +1,7 @@ package config import ( + "errors" "os" "path/filepath" "reflect" @@ -165,3 +166,143 @@ func TestStoreValueUpdatesConf(t *testing.T) { t.Errorf("FTSMock.Interval = %v; want 30s", conf.Sources.FTSMock.Interval) } } + +// parseValue parses the kinds a key can have; no current key is an integer +// or float, so StoreValue can't reach those cases yet. +func TestParseValue(t *testing.T) { + for _, tt := range []struct { + kind reflect.Kind + value string + want any + }{ + {reflect.Bool, "true", true}, + {reflect.String, "text", "text"}, + {reflect.Int, "-42", int64(-42)}, + {reflect.Int8, "7", int64(7)}, + {reflect.Uint16, "65535", uint64(65535)}, + {reflect.Float32, "1.5", 1.5}, + } { + got, err := parseValue(tt.kind, tt.value) + if err != nil || got != tt.want { + t.Errorf("parseValue(%v, %q) = %#v, %v; want %#v", tt.kind, tt.value, got, err, tt.want) + } + } + + for _, tt := range []struct { + kind reflect.Kind + value string + }{ + {reflect.Bool, "maybe"}, + {reflect.Int, "1.5"}, + {reflect.Uint, "-1"}, + {reflect.Float64, "x"}, + } { + if got, err := parseValue(tt.kind, tt.value); err == nil { + t.Errorf("parseValue(%v, %q) = %#v; want an error", tt.kind, tt.value, got) + } + } + + if _, err := parseValue(reflect.Slice, "a,b"); !errors.Is(err, errors.ErrUnsupported) { + t.Errorf("parseValue(slice) err = %v; want ErrUnsupported", err) + } +} + +// numberSection has the numeric kinds JobSearchConfig doesn't use yet, to +// test storeValue with them. +type numberSection struct { + Count int `cfg:"count"` + Small int8 `cfg:"small"` + Big uint16 `cfg:"big"` + Ratio float32 `cfg:"ratio"` +} + +func (c numberSection) Map() map[string]any { return mapOf(c) } +func (c numberSection) Flatten() map[string]any { return flattenOf("", c) } +func (c numberSection) TypeOf(key string) (reflect.Kind, error) { return typeOf(c, key) } +func (c numberSection) ValueOf(key string) (any, error) { return valueOf(c, key) } +func (c *numberSection) SetValue(key string, value any) error { return setValue(c, key, value) } + +// Numbers are stored as the field's own type, not as the int64, uint64 or +// float64 that parsing produces. +func TestStoreValueNumbers(t *testing.T) { + for _, tt := range []struct { + key, value string + want any + kind reflect.Kind + }{ + {"count", "-42", -42, reflect.Int}, + {"small", "7", int8(7), reflect.Int8}, + {"big", "65535", uint16(65535), reflect.Uint16}, + {"ratio", "1.5", float32(1.5), reflect.Float32}, + } { + t.Run(tt.key, func(t *testing.T) { + path := useConfigFile(t, "count: 1\n") + var s numberSection + + got, kind, err := storeValue(&s, tt.key, tt.value) + if err != nil { + t.Fatal(err) + } + if got != tt.want || kind != tt.kind { + t.Errorf("got %#v <%s>; want %#v <%s>", got, kind, tt.want, tt.kind) + } + if v, _ := s.ValueOf(tt.key); v != tt.want { + t.Errorf("section has %s = %#v; want %#v", tt.key, v, tt.want) + } + if v := readConfigFile(t, path).GetString(tt.key); v != tt.value { + t.Errorf("file has %s = %q; want %q", tt.key, v, tt.value) + } + }) + } + + for key, value := range map[string]string{ + "count": "1.5", // not an integer + "small": "300", // too big for an int8 + "big": "-1", // negative + "ratio": "lots", // not a number + } { + useConfigFile(t, "count: 1\n") + if _, _, err := storeValue(&numberSection{}, key, value); err == nil { + t.Errorf("storeValue(%s, %q) succeeded", key, value) + } + } +} + +// Only the file's own settings are written back: defaults (and environment +// variables) loaded into Viper stay out of the file. +func TestStoreAndPurgeWriteOnlyFileSettings(t *testing.T) { + path := useConfigFile(t, "logging:\n level: warn\n file: /tmp/js.log\n") + LoadDefaultConfig() + conf := defaultConfig(t) + + if _, _, err := StoreValue(&conf, "sources.sleeper.enabled", "true"); err != nil { + t.Fatal(err) + } + want := map[string]any{ + "logging.level": "warn", + "logging.file": "/tmp/js.log", + "sources.sleeper.enabled": true, + } + checkFileSettings(t, path, want) + + if err := PurgeValue(&conf, "logging.file"); err != nil { + t.Fatal(err) + } + delete(want, "logging.file") + checkFileSettings(t, path, want) +} + +// checkFileSettings checks the config file at path holds exactly want. +func checkFileSettings(t *testing.T, path string, want map[string]any) { + t.Helper() + v := readConfigFile(t, path) + got := map[string]any{} + for _, k := range v.AllKeys() { + got[k] = v.Get(k) + } + if !reflect.DeepEqual(got, want) { + //nolint:gosec // G304: reading a file the test created in t.TempDir + b, _ := os.ReadFile(path) + t.Errorf("file settings = %v; want %v\n%s", got, want, b) + } +} diff --git a/internal/config/print.go b/internal/config/print.go index 698b008..99798bb 100644 --- a/internal/config/print.go +++ b/internal/config/print.go @@ -3,6 +3,8 @@ package config import ( "fmt" "log/slog" + "maps" + "slices" ) // PrintTerminal prints the configuration to stdout as String formats it. @@ -20,8 +22,8 @@ func (conf JobSearchConfig) PrintYaml() { fmt.Print(conf.YAML() + "\n") } -// PrintLog logs each configuration key and value at atLevel: debug, warn, -// error, or info for any other level. +// PrintLog logs each configuration key and value, sorted by key, at atLevel: +// debug, warn, error, or info for any other level. func (conf JobSearchConfig) PrintLog(atLevel slog.Level) { var logFn func(args ...interface{}) @@ -44,7 +46,8 @@ func (conf JobSearchConfig) PrintLog(atLevel slog.Level) { } } - for k, v := range conf.Flatten() { - logFn("\t", k, " = ", v) + flat := conf.Flatten() + for _, k := range slices.Sorted(maps.Keys(flat)) { + logFn("\t", k, " = ", flat[k]) } } diff --git a/internal/config/print_test.go b/internal/config/print_test.go new file mode 100644 index 0000000..fe7d924 --- /dev/null +++ b/internal/config/print_test.go @@ -0,0 +1,129 @@ +package config + +import ( + "encoding/json" + "log/slog" + "os" + "path/filepath" + "slices" + "strings" + "testing" + + "go.yaml.in/yaml/v3" +) + +// captureStdout returns what fn prints to os.Stdout. +func captureStdout(t *testing.T, fn func()) string { + t.Helper() + path := filepath.Join(t.TempDir(), "stdout") + f, err := os.Create(filepath.Clean(path)) + if err != nil { + t.Fatal(err) + } + old := os.Stdout + os.Stdout = f + fn() + os.Stdout = old + if err := f.Close(); err != nil { + t.Fatal(err) + } + b, err := os.ReadFile(filepath.Clean(path)) + if err != nil { + t.Fatal(err) + } + return string(b) +} + +func TestPrintTerminal(t *testing.T) { + conf := defaultConfig(t) + got := captureStdout(t, conf.PrintTerminal) + if want := conf.String() + "\n"; got != want { + t.Errorf("PrintTerminal printed %q; want %q", got, want) + } +} + +// String lists every key once, sorted, so the output is the same each run. +func TestStringSorted(t *testing.T) { + conf := defaultConfig(t) + var keys []string + for _, line := range strings.Split(strings.TrimSpace(conf.String()), "\n") { + key, _, _ := strings.Cut(line, " = ") + keys = append(keys, key) + } + if !slices.IsSorted(keys) || len(keys) != len(conf.Flatten()) { + t.Errorf("String keys = %v; want each key once, sorted", keys) + } + if first := conf.String(); conf.String() != first { + t.Error("String output differs between calls") + } +} + +func TestPrintJson(t *testing.T) { + conf := defaultConfig(t) + out := captureStdout(t, conf.PrintJson) + var m map[string]any + if err := json.Unmarshal([]byte(out), &m); err != nil { + t.Fatalf("PrintJson output is not JSON: %v\n%s", err, out) + } + if got := m["logging"].(map[string]any)["level"]; got != "warn" { + t.Errorf("logging.level = %v; want warn", got) + } +} + +func TestPrintYaml(t *testing.T) { + conf := defaultConfig(t) + out := captureStdout(t, conf.PrintYaml) + var m map[string]any + if err := yaml.Unmarshal([]byte(out), &m); err != nil { + t.Fatalf("PrintYaml output is not YAML: %v\n%s", err, out) + } + if got := m["logging"].(map[string]any)["level"]; got != "warn" { + t.Errorf("logging.level = %v; want warn", got) + } +} + +// PrintLog logs one line per key, at the level asked for (info for any +// level other than debug, warn and error). +func TestPrintLog(t *testing.T) { + conf := defaultConfig(t) + for _, tt := range []struct { + at slog.Level + want string + }{ + {slog.LevelDebug, "level=DEBUG"}, + {slog.LevelInfo, "level=INFO"}, + {slog.LevelWarn, "level=WARN"}, + {slog.LevelError, "level=ERROR"}, + {slog.LevelError + 4, "level=INFO"}, + } { + logs := captureLogs(t, slog.LevelDebug) + conf.PrintLog(tt.at) + + lines := strings.Split(strings.TrimSpace(logs.String()), "\n") + if len(lines) != len(conf.Flatten()) { + t.Errorf("PrintLog(%v) logged %d lines; want %d", tt.at, len(lines), len(conf.Flatten())) + } + for _, l := range lines { + if !strings.Contains(l, tt.want) { + t.Errorf("PrintLog(%v) line %q; want %s", tt.at, l, tt.want) + } + } + if !strings.Contains(logs.String(), "logging.level = warn") { + t.Errorf("PrintLog(%v) missing logging.level:\n%s", tt.at, logs) + } + // Each line is msg="\t = ". + flat := conf.Flatten() + var keys []string + for _, l := range lines { + _, msg, _ := strings.Cut(l, `msg="\t`) + key, _, _ := strings.Cut(msg, " = ") + if _, ok := flat[key]; !ok { + t.Errorf("PrintLog(%v) line %q: no key", tt.at, l) + } + keys = append(keys, key) + } + if !slices.IsSorted(keys) { + t.Errorf("PrintLog(%v) keys not sorted: %v", tt.at, keys) + } + } +} diff --git a/internal/config/section_test.go b/internal/config/section_test.go index ab5c6fb..bb16bef 100644 --- a/internal/config/section_test.go +++ b/internal/config/section_test.go @@ -301,3 +301,12 @@ func TestSetValueValidator(t *testing.T) { t.Errorf("nested setValue(bad) = %v; want errRejected", err) } } + +// parseString handles string fields too, although convert assigns a string +// to a string field before reaching it. +func TestParseStringString(t *testing.T) { + v, err := parseString("text", reflect.TypeOf("")) + if err != nil || v.Interface() != "text" { + t.Errorf("parseString(text, string) = %v, %v", v, err) + } +}