config updated to improve setting values, writing changes and test coverage

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`.
 

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)
+		}
+	}
+}

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 <type>" 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()
 }

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
 }

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)
+	}
+}

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])
 	}
 }

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<key> = <value>".
+		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)
+		}
+	}
+}

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)
+	}
+}