Stop startup on a list or map setting in neoirc.yaml (closes #268)

Every setting is now read through one helper that uses cast.ToStringE,
the conversion viper.GetString wraps, keeping its error instead of
returning the empty string. TRUSTED_PROXIES uses the same helper. A
config file that does not parse makes New return an error naming the
file instead of panicking.

Model: opus-5-5

internal/config/config.go

@@ -12,6 +12,7 @@ import (
 	"strings"
 	"time"
 
+	"github.com/spf13/cast"
 	"github.com/spf13/viper"
 	"go.uber.org/fx"
 	"go4.org/netipx"
@@ -43,7 +44,7 @@ var errOutOfRange = errors.New("setting out of range")
 
 var errNeverMatches = errors.New("entry can never match a connection")
 
-var errNotString = errors.New("not a string")
+var errListOrMap = errors.New("a setting cannot be a list or a map")
 
 // Params defines the dependencies for creating a Config.
 type Params struct {
@@ -81,8 +82,9 @@ type Config struct {
 }
 
 // New creates a new Config by reading from files and environment variables.
-// It returns an error when a number, true/false, duration or address
-// setting does not parse or is out of range.
+// It returns an error when the config file does not parse, when the file
+// gives a setting a list or a map, and when a number, true/false,
+// duration or address setting does not parse or is out of range.
 func New(
 	_ fx.Lifecycle, params Params,
 ) (*Config, error) {
@@ -121,27 +123,14 @@ func New(
 	if err != nil {
 		var notFound viper.ConfigFileNotFoundError
 		if !errors.As(err, &notFound) {
-			log.Error("config file malformed", "error", err)
-			panic(err)
+			return nil, fmt.Errorf("cannot read %s: %w",
+				viper.ConfigFileUsed(), err)
 		}
 	}
 
 	cfg := &Config{
-		DBURL:              viper.GetString("DBURL"),
-		SentryDSN:          viper.GetString("SENTRY_DSN"),
-		MetricsUsername:    viper.GetString("METRICS_USERNAME"),
-		MetricsPassword:    viper.GetString("METRICS_PASSWORD"),
-		MessageMaxAge:      viper.GetString("MESSAGE_MAX_AGE"),
-		QueueMaxAge:        viper.GetString("QUEUE_MAX_AGE"),
-		MOTD:               viper.GetString("MOTD"),
-		ServerName:         viper.GetString("SERVER_NAME"),
-		FederationKey:      viper.GetString("FEDERATION_KEY"),
-		SessionIdleTimeout: viper.GetString("SESSION_IDLE_TIMEOUT"),
-		OperName:           viper.GetString("NEOIRC_OPER_NAME"),
-		OperPassword:       viper.GetString("NEOIRC_OPER_PASSWORD"),
-		IRCListenAddr:      viper.GetString("IRC_LISTEN_ADDR"),
-		log:                log,
-		params:             &params,
+		log:    log,
+		params: &params,
 	}
 
 	err = parseSettings(cfg)
@@ -164,13 +153,23 @@ func New(
 	return cfg, nil
 }
 
-// parseSettings reads the number, true/false and address settings into
-// cfg and checks the duration settings. Each number and duration must be
-// above zero. PORT and NEOIRC_HASHCASH_BITS may also be zero: port 0 lets
-// the system pick a port, and 0 bits turns proof of work off. The error
-// names each setting that does not parse or is out of range.
+// parseSettings reads every setting into cfg. Each number and duration
+// must be above zero. PORT and NEOIRC_HASHCASH_BITS may also be zero:
+// port 0 lets the system pick a port, and 0 bits turns proof of work off.
+// The error names each setting that is a list or a map, does not parse or
+// is out of range.
 func parseSettings(cfg *Config) error {
 	return errors.Join(
+		parseString("DBURL", &cfg.DBURL),
+		parseString("SENTRY_DSN", &cfg.SentryDSN),
+		parseString("METRICS_USERNAME", &cfg.MetricsUsername),
+		parseString("METRICS_PASSWORD", &cfg.MetricsPassword),
+		parseString("MOTD", &cfg.MOTD),
+		parseString("SERVER_NAME", &cfg.ServerName),
+		parseString("FEDERATION_KEY", &cfg.FederationKey),
+		parseString("NEOIRC_OPER_NAME", &cfg.OperName),
+		parseString("NEOIRC_OPER_PASSWORD", &cfg.OperPassword),
+		parseString("IRC_LISTEN_ADDR", &cfg.IRCListenAddr),
 		parseInt("PORT", 0, &cfg.Port),
 		parseInt("MAX_MESSAGE_SIZE", 1, &cfg.MaxMessageSize),
 		parseInt("NEOIRC_HASHCASH_BITS", 0, &cfg.HashcashBits),
@@ -178,39 +177,61 @@ func parseSettings(cfg *Config) error {
 		parseFloat("LOGIN_RATE_LIMIT", &cfg.LoginRateLimit),
 		parseBool("DEBUG", &cfg.Debug),
 		parseBool("MAINTENANCE_MODE", &cfg.MaintenanceMode),
-		checkDuration("MESSAGE_MAX_AGE", cfg.MessageMaxAge),
-		checkDuration("QUEUE_MAX_AGE", cfg.QueueMaxAge),
-		checkDuration("SESSION_IDLE_TIMEOUT", cfg.SessionIdleTimeout),
+		parseDuration("MESSAGE_MAX_AGE", &cfg.MessageMaxAge),
+		parseDuration("QUEUE_MAX_AGE", &cfg.QueueMaxAge),
+		parseDuration("SESSION_IDLE_TIMEOUT", &cfg.SessionIdleTimeout),
 		parseTrustedProxies(&cfg.TrustedProxies),
 	)
 }
 
+// getString returns the setting key as a string. A number or a true/false
+// value in the config file becomes its text, as with viper.GetString. A
+// list or a map in the config file is an error, where viper.GetString
+// would return the empty string.
+func getString(key string) (string, error) {
+	value := viper.Get(key)
+
+	raw, err := cast.ToStringE(value)
+	if err != nil {
+		return "", fmt.Errorf("%w: %s is %v", errListOrMap, key, value)
+	}
+
+	return raw, nil
+}
+
+// parseString sets *value to the setting key.
+func parseString(key string, value *string) error {
+	raw, err := getString(key)
+	if err != nil {
+		return err
+	}
+
+	*value = raw
+
+	return nil
+}
+
 // parseTrustedProxies sets *value to the addresses in TRUSTED_PROXIES, a
 // comma-separated list of networks such as 10.0.0.0/8 and single
 // addresses such as 192.0.2.1. The empty string trusts nothing. Viper
 // counts an empty environment variable as unset, so the environment is
 // read directly as well.
 func parseTrustedProxies(value **netipx.IPSet) error {
-	setting := viper.Get("TRUSTED_PROXIES")
+	raw, err := getString("TRUSTED_PROXIES")
+	if err != nil {
+		return err
+	}
 
 	fromEnv, set := os.LookupEnv("TRUSTED_PROXIES")
 	if set {
-		setting = fromEnv
-	}
-
-	// A YAML list or map in the config file is not a string, and
-	// viper.GetString would turn it into the empty string.
-	raw, isString := setting.(string)
-	if !isString {
-		return fmt.Errorf("cannot parse TRUSTED_PROXIES: %v is %w",
-			setting, errNotString)
+		raw = fromEnv
 	}
 
 	var builder netipx.IPSetBuilder
 
 	if strings.TrimSpace(raw) != "" {
 		for entry := range strings.SplitSeq(raw, ",") {
-			err := addTrustedProxy(&builder, strings.TrimSpace(entry))
+			err = addTrustedProxy(&builder, strings.TrimSpace(entry))
 			if err != nil {
 				return fmt.Errorf("cannot parse TRUSTED_PROXIES: %w", err)
 			}
@@ -267,7 +288,10 @@ func addTrustedProxy(builder *netipx.IPSetBuilder, entry string) error {
 // parseInt sets *value to the setting key read as a whole number. It
 // returns an error when the value does not parse or is below minimum.
 func parseInt(key string, minimum int, value *int) error {
-	raw := viper.GetString(key)
+	raw, err := getString(key)
+	if err != nil {
+		return err
+	}
 
 	parsed, err := strconv.Atoi(raw)
 	if err != nil {
@@ -288,7 +312,10 @@ func parseInt(key string, minimum int, value *int) error {
 // number. It returns an error when the value does not parse or is not a
 // finite number above zero.
 func parseFloat(key string, value *float64) error {
-	raw := viper.GetString(key)
+	raw, err := getString(key)
+	if err != nil {
+		return err
+	}
 
 	parsed, err := strconv.ParseFloat(raw, 64)
 	if err != nil {
@@ -308,7 +335,12 @@ func parseFloat(key string, value *float64) error {
 
 // parseBool sets *value to the setting key read as true or false.
 func parseBool(key string, value *bool) error {
-	parsed, err := strconv.ParseBool(viper.GetString(key))
+	raw, err := getString(key)
+	if err != nil {
+		return err
+	}
+
+	parsed, err := strconv.ParseBool(raw)
 	if err != nil {
 		return fmt.Errorf("cannot parse %s: %w", key, err)
 	}
@@ -318,18 +350,25 @@ func parseBool(key string, value *bool) error {
 	return nil
 }
 
-// checkDuration returns an error naming the setting key when value is
-// not a Go duration above zero.
-func checkDuration(key, value string) error {
-	duration, err := time.ParseDuration(value)
+// parseDuration sets *value to the setting key. It returns an error when
+// the value is not a Go duration above zero.
+func parseDuration(key string, value *string) error {
+	raw, err := getString(key)
+	if err != nil {
+		return err
+	}
+
+	duration, err := time.ParseDuration(raw)
 	if err != nil {
 		return fmt.Errorf("cannot parse %s: %w", key, err)
 	}
 
 	if duration <= 0 {
 		return fmt.Errorf("%w: %s is %s, but must be above zero",
-			errOutOfRange, key, value)
+			errOutOfRange, key, raw)
 	}
 
+	*value = raw
+
 	return nil
 }

internal/config/config_test.go

@@ -287,49 +287,93 @@ func TestTrustedProxies(t *testing.T) {
 	}
 }
 
-func TestTrustedProxiesNotAStringInYAMLStopsStartup(t *testing.T) {
-	// YAML reads the first value as a list and the second as a map.
-	values := []string{"[10.0.0.1]", "{proxy: 10.0.0.1}"}
+// unsetEnv removes the environment variable key for the rest of the
+// test, so that config.New reads key from the config file.
+func unsetEnv(t *testing.T, key string) {
+	t.Helper()
 
-	for _, value := range values {
-		t.Run(value, func(t *testing.T) {
-			// t.Setenv puts back whatever value was there after the
-			// test, including when Unsetenv removes it.
-			t.Setenv("TRUSTED_PROXIES", "")
+	// t.Setenv puts back whatever value was there after the test,
+	// including when Unsetenv removes it.
+	t.Setenv(key, "")
 
-			err := os.Unsetenv("TRUSTED_PROXIES")
-			if err != nil {
-				t.Fatal(err)
-			}
-
-			// loadConfig names the app neoirc-test, so config.New reads
-			// $HOME/.config/neoirc-test/neoirc-test.yaml.
-			home := t.TempDir()
-			t.Setenv("HOME", home)
-
-			dir := filepath.Join(home, ".config", "neoirc-test")
-
-			err = os.MkdirAll(dir, 0o700)
-			if err != nil {
-				t.Fatal(err)
-			}
-
-			err = os.WriteFile(
-				filepath.Join(dir, "neoirc-test.yaml"),
-				[]byte("TRUSTED_PROXIES: "+value+"\n"), 0o600,
-			)
-			if err != nil {
-				t.Fatal(err)
-			}
-
-			// Viper keeps the values it read from this file after the
-			// file is gone. Reset clears them for later tests.
-			t.Cleanup(viper.Reset)
-
-			_, err = loadConfig(t)
-			if err == nil || !strings.Contains(err.Error(), "TRUSTED_PROXIES") {
-				t.Errorf("error = %v, want one naming TRUSTED_PROXIES", err)
-			}
-		})
+	err := os.Unsetenv(key)
+	if err != nil {
+		t.Fatal(err)
+	}
+}
+
+// writeConfigFile writes content to the config file config.New reads
+// when HOME is home, and returns the file's path.
+func writeConfigFile(t *testing.T, home, content string) string {
+	t.Helper()
+
+	// loadConfig names the app neoirc-test, so config.New reads
+	// $HOME/.config/neoirc-test/neoirc-test.yaml.
+	dir := filepath.Join(home, ".config", "neoirc-test")
+
+	err := os.MkdirAll(dir, 0o700)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	path := filepath.Join(dir, "neoirc-test.yaml")
+
+	err = os.WriteFile(path, []byte(content), 0o600)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// Viper keeps the file and the values it read from it after the
+	// file is gone. Reset clears them for later tests.
+	t.Cleanup(viper.Reset)
+
+	return path
+}
+
+func TestListOrMapInConfigFileStopsStartup(t *testing.T) {
+	// YAML reads the first value as a list and the second as a map.
+	values := []string{`[":6667"]`, `{addr: ":6667"}`}
+	keys := []string{"IRC_LISTEN_ADDR", "MOTD", "TRUSTED_PROXIES"}
+
+	for _, key := range keys {
+		for _, value := range values {
+			t.Run(key+"="+value, func(t *testing.T) {
+				home := t.TempDir()
+				t.Setenv("HOME", home)
+				unsetEnv(t, key)
+				writeConfigFile(t, home, key+": "+value+"\n")
+
+				_, err := loadConfig(t)
+				if err == nil || !strings.Contains(err.Error(), key) {
+					t.Errorf("error = %v, want one naming %s", err, key)
+				}
+			})
+		}
+	}
+}
+
+func TestSingleValuesInConfigFileAreUsed(t *testing.T) {
+	home := t.TempDir()
+	t.Setenv("HOME", home)
+	unsetEnv(t, "PORT")
+	unsetEnv(t, "DEBUG")
+	unsetEnv(t, "MOTD")
+	writeConfigFile(t, home, "PORT: 9000\nDEBUG: true\nMOTD: hello\n")
+
+	cfg := newConfig(t)
+	if cfg.Port != 9000 || !cfg.Debug || cfg.MOTD != "hello" {
+		t.Errorf("Port, Debug, MOTD = %d, %t, %q, want 9000, true, %q",
+			cfg.Port, cfg.Debug, cfg.MOTD, "hello")
+	}
+}
+
+func TestConfigFileThatDoesNotParseStopsStartup(t *testing.T) {
+	home := t.TempDir()
+	t.Setenv("HOME", home)
+	path := writeConfigFile(t, home, "MOTD: [unclosed\n")
+
+	_, err := loadConfig(t)
+	if err == nil || !strings.Contains(err.Error(), path) {
+		t.Errorf("error = %v, want one naming %s", err, path)
 	}
 }