diff --git a/conf/configuration.go b/conf/configuration.go index 3d04e725..653e3d8e 100644 --- a/conf/configuration.go +++ b/conf/configuration.go @@ -258,10 +258,10 @@ type searchOptions struct { FullString bool } -// fatalFunc is called for fatal config errors. Defaults to printing + os.Exit(1). +// logFatal prints a fatal error message to stderr and exits. // Overridden in tests to allow testing fatal paths. -var fatalFunc = func(msg string) { - _, _ = fmt.Fprintln(os.Stderr, "FATAL:", msg) +var logFatal = func(args ...any) { + _, _ = fmt.Fprintln(os.Stderr, append([]any{"FATAL:"}, args...)...) os.Exit(1) } @@ -274,8 +274,7 @@ func LoadFromFile(confFile string) { viper.SetConfigFile(confFile) err := viper.ReadInConfig() if err != nil { - _, _ = fmt.Fprintln(os.Stderr, "FATAL: Error reading config file:", err) - os.Exit(1) + logFatal("Error reading config file:", err) } Load(true) } @@ -292,14 +291,12 @@ func Load(noConfigDump bool) { err := viper.Unmarshal(&Server) if err != nil { - _, _ = fmt.Fprintln(os.Stderr, "FATAL: Error parsing config:", err) - os.Exit(1) + logFatal("Error parsing config:", err) } err = os.MkdirAll(Server.DataFolder, os.ModePerm) if err != nil { - _, _ = fmt.Fprintln(os.Stderr, "FATAL: Error creating data path:", err) - os.Exit(1) + logFatal("Error creating data path:", err) } if Server.CacheFolder == "" { @@ -307,14 +304,12 @@ func Load(noConfigDump bool) { } err = os.MkdirAll(Server.CacheFolder, os.ModePerm) if err != nil { - _, _ = fmt.Fprintln(os.Stderr, "FATAL: Error creating cache path:", err) - os.Exit(1) + logFatal("Error creating cache path:", err) } err = os.MkdirAll(filepath.Join(Server.DataFolder, consts.ArtworkFolder), os.ModePerm) if err != nil { - _, _ = fmt.Fprintln(os.Stderr, "FATAL: Error creating artwork path:", err) - os.Exit(1) + logFatal("Error creating artwork path:", err) } if Server.Plugins.Enabled { @@ -323,8 +318,7 @@ func Load(noConfigDump bool) { } err = os.MkdirAll(Server.Plugins.Folder, 0700) if err != nil { - _, _ = fmt.Fprintln(os.Stderr, "FATAL: Error creating plugins path:", err) - os.Exit(1) + logFatal("Error creating plugins path:", err) } } @@ -336,8 +330,7 @@ func Load(noConfigDump bool) { if Server.Backup.Path != "" { err = os.MkdirAll(Server.Backup.Path, os.ModePerm) if err != nil { - _, _ = fmt.Fprintln(os.Stderr, "FATAL: Error creating backup path:", err) - os.Exit(1) + logFatal("Error creating backup path:", err) } } @@ -345,8 +338,7 @@ func Load(noConfigDump bool) { if Server.LogFile != "" { out, err = os.OpenFile(Server.LogFile, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644) if err != nil { - _, _ = fmt.Fprintf(os.Stderr, "FATAL: Error opening log file %s: %s\n", Server.LogFile, err.Error()) - os.Exit(1) + logFatal(fmt.Sprintf("Error opening log file %s: %s", Server.LogFile, err.Error())) } log.SetOutput(out) } else if os.Getenv("ND_SYSTEMD_PRIORITY_LOGGING") != "" && os.Getenv("JOURNAL_STREAM") != "" { @@ -378,8 +370,7 @@ func Load(noConfigDump bool) { if Server.BaseURL != "" { u, err := url.Parse(Server.BaseURL) if err != nil { - _, _ = fmt.Fprintln(os.Stderr, "FATAL: Invalid BaseURL:", err) - os.Exit(1) + logFatal("Invalid BaseURL:", err) } Server.BasePath = u.Path u.Path = "" @@ -487,7 +478,7 @@ func remapEnvVarKeysFromConfig() { displayCanonical := toPascalCase(canonicalKey) if viper.InConfig(canonicalKey) { - fatalFunc(fmt.Sprintf( + logFatal(fmt.Sprintf( "Config file contains both '%s' and '%s'. Remove the ND_-prefixed version. "+ "The 'ND_' prefix is only needed for environment variables, not config file keys.", displayNDKey, displayCanonical, @@ -520,18 +511,15 @@ func parseIniFileConfiguration() { var iniConfig map[string]any err := viper.Unmarshal(&iniConfig) if err != nil { - _, _ = fmt.Fprintln(os.Stderr, "FATAL: Error parsing config:", err) - os.Exit(1) + logFatal("Error parsing config:", err) } cfg, ok := iniConfig["default"].(map[string]any) if !ok { - _, _ = fmt.Fprintln(os.Stderr, "FATAL: Error parsing config: missing [default] section:", iniConfig) - os.Exit(1) + logFatal("Error parsing config: missing [default] section:", iniConfig) } err = viper.MergeConfigMap(cfg) if err != nil { - _, _ = fmt.Fprintln(os.Stderr, "FATAL: Error parsing config:", err) - os.Exit(1) + logFatal("Error parsing config:", err) } } } @@ -872,8 +860,7 @@ func InitConfig(cfgFile string, loadEnvVars bool) { err := viper.ReadInConfig() if viper.ConfigFileUsed() != "" && err != nil { - _, _ = fmt.Fprintln(os.Stderr, "FATAL: Navidrome could not open config file: ", err) - os.Exit(1) + logFatal("Navidrome could not open config file:", err) } } diff --git a/conf/configuration_test.go b/conf/configuration_test.go index 9aba0419..eb2176e8 100644 --- a/conf/configuration_test.go +++ b/conf/configuration_test.go @@ -2,6 +2,7 @@ package conf_test import ( "fmt" + "os" "path/filepath" "testing" @@ -24,6 +25,11 @@ var _ = Describe("Configuration", func() { viper.SetDefault("datafolder", GinkgoT().TempDir()) viper.SetDefault("loglevel", "error") conf.ResetConf() + + // Panic instead of exiting on fatal errors to allow testing error conditions + DeferCleanup(conf.SetLogFatal(func(args ...any) { + panic(fmt.Sprint(args...)) + })) }) Describe("ParseLanguages", func() { @@ -139,11 +145,6 @@ var _ = Describe("Configuration", func() { }) It("exits with fatal error when both ND_ and canonical key exist", func() { - cleanup := conf.SetFatalFunc(func(msg string) { - panic(msg) - }) - defer cleanup() - filename := filepath.Join("testdata", "cfg_nd_conflict.toml") conf.InitConfig(filename, false) @@ -164,6 +165,60 @@ var _ = Describe("Configuration", func() { }) }) + Describe("logFatal", func() { + var invalidPath string + BeforeEach(func() { + viper.Reset() + conf.SetViperDefaults() + viper.SetDefault("loglevel", "error") + conf.ResetConf() + + // Create a file so that any path under it is invalid on all OSes + f, err := os.CreateTemp(GinkgoT().TempDir(), "blocker") + Expect(err).ToNot(HaveOccurred()) + f.Close() + invalidPath = filepath.Join(f.Name(), "subdir") + }) + + It("is called when LoadFromFile gets an invalid config file", func() { + Expect(func() { + conf.LoadFromFile(filepath.Join(invalidPath, "file.toml")) + }).To(PanicWith(ContainSubstring("Error reading config file"))) + }) + + It("is called when DataFolder is not writable", func() { + viper.SetDefault("datafolder", invalidPath) + Expect(func() { + conf.Load(true) + }).To(PanicWith(ContainSubstring("Error creating data path"))) + }) + + It("is called when CacheFolder is not writable", func() { + viper.SetDefault("datafolder", GinkgoT().TempDir()) + viper.SetDefault("cachefolder", invalidPath) + Expect(func() { + conf.Load(true) + }).To(PanicWith(ContainSubstring("Error creating cache path"))) + }) + + It("is called when LogFile path is not writable", func() { + viper.SetDefault("datafolder", GinkgoT().TempDir()) + viper.SetDefault("logfile", filepath.Join(invalidPath, "log.txt")) + Expect(func() { + conf.Load(true) + }).To(PanicWith(ContainSubstring("Error opening log file"))) + }) + + It("is called when BaseURL is invalid", func() { + viper.SetDefault("datafolder", GinkgoT().TempDir()) + viper.SetDefault("baseurl", "://invalid") + Expect(func() { + conf.Load(true) + }).To(PanicWith(ContainSubstring("Invalid BaseURL"))) + }) + + }) + DescribeTable("should load configuration from", func(format string) { filename := filepath.Join("testdata", "cfg."+format) diff --git a/conf/export_test.go b/conf/export_test.go index 6498215d..051f9bb6 100644 --- a/conf/export_test.go +++ b/conf/export_test.go @@ -14,8 +14,8 @@ var NormalizeSearchBackend = normalizeSearchBackend var ToPascalCase = toPascalCase -func SetFatalFunc(f func(string)) func() { - old := fatalFunc - fatalFunc = f - return func() { fatalFunc = old } +func SetLogFatal(f func(...any)) func() { + old := logFatal + logFatal = f + return func() { logFatal = old } }