feat(config): warn about unrecognized options in the config file (#5870)

* feat(config): warn about unrecognized options in the config file

Options that don't match any known name were silently discarded, so a typo
or an option written outside its section looked like it was applied.

In #5869 the user set `ArtistSplitExceptions` at the root level instead of
under `Scanner`, and got no feedback that the option was being ignored.

The known names are derived by reflection over configOptions, so the check
stays in sync with the struct. Free-form maps (Tags, DevLogLevels) accept any
subkey. When an unknown key matches the last segment of a known one, the
warning suggests it. Keys are reported as spelled in the config file,
recovered by scanning it, since viper lowercases every key it loads.

Also fixes two gaps this surfaced:

- remapEnvVarKeysFromConfig accepted any ND_-prefixed key and advised a
  canonical name built by string substitution, so `ND_SCANNER_WATCHERENABLED`
  (not an option) suggested `Scanner.Watcherenabled` (not an option either).
  It now only advises names that exist, with their documented spelling, and
  leaves the rest to the unrecognized-option warning.

- The deprecated option list drove only the warnings, while the value
  migration kept a second hardcoded list. They had drifted: SearchFullString
  warned about `Search.FullString` but never migrated to it. Both now come
  from deprecatedOptions.

* fix(config): address Codex review on the unrecognized-option warning

- Values computed during Load (ConfigFile, LastFM.Languages, Deezer.Languages)
  were accepted as valid keys, so setting them in the config file stayed silent
  even though Load overwrites them. They are now marked `conf:"-"` at the
  declaration, so the exclusion can't drift from the struct.

- Removed options are in the known-key set only so they get their own warning,
  but suggestOptions drew from the same set, so an unknown `ID` advised
  `Spotify.ID`, a key Navidrome explicitly ignores. They are now filtered out
  of suggestions.

- mapDeprecatedOption uses viper.Set, which outranks the config file, so a
  deprecated value overrode an explicitly configured replacement. It now skips
  the migration when the replacement was provided. viper.IsSet counts defaults
  as set, so the check is InConfig plus the env var.

envVarName also returns "" for an empty option, so a deprecated option with no
replacement no longer advises "Please use the new 'ND_'".

* fix(config): cover the ND_ spelling of a replacement, and the warning output

- explicitlySet missed the case where the replacement is given in the config
  file under its ND_ spelling: remapEnvVarKeysFromConfig moves it to the
  override layer, out of InConfig's reach, so the deprecated value still won.
  It now also checks the ND_-prefixed config key.

- The tests asserted only the helpers' return values, so removing the
  logUnknownOptions call from Load left them green. Added a spec that captures
  the logger and checks the emitted warning and suggestion text; verified it
  fails when the call is removed.
This commit is contained in:
Deluan Quintão 2026-07-29 13:44:42 -04:00 committed by GitHub
parent 23548f40a0
commit add0a6dc9b
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
12 changed files with 380 additions and 35 deletions

View File

@ -2,14 +2,18 @@ package conf
import (
"cmp"
"encoding"
"encoding/json"
"fmt"
"net/url"
"os"
"path/filepath"
"reflect"
"regexp"
"runtime"
"slices"
"strings"
"sync"
"time"
"github.com/bmatcuk/doublestar/v4"
@ -21,11 +25,12 @@ import (
"github.com/navidrome/navidrome/log"
"github.com/navidrome/navidrome/scheduler"
"github.com/navidrome/navidrome/utils/run"
"github.com/navidrome/navidrome/utils/slice"
"github.com/spf13/viper"
)
type configOptions struct {
ConfigFile string
ConfigFile string `conf:"-"`
Address string
Port int
UnixSocketPerm string
@ -201,7 +206,7 @@ type lastfmOptions struct {
ScrobbleFirstArtistOnly bool
// Computed values
Languages []string // Computed from Language, split by comma
Languages []string `conf:"-"` // Computed from Language, split by comma
}
type deezerOptions struct {
@ -209,7 +214,7 @@ type deezerOptions struct {
Language string
// Computed values
Languages []string // Computed from Language, split by comma
Languages []string `conf:"-"` // Computed from Language, split by comma
}
type listenBrainzOptions struct {
@ -340,12 +345,11 @@ func Load(noConfigDump bool) {
remapEnvVarKeysFromConfig()
// Map deprecated options to their new names for backwards compatibility
mapDeprecatedOption("ReverseProxyWhitelist", "ExtAuth.TrustedSources")
mapDeprecatedOption("ReverseProxyUserHeader", "ExtAuth.UserHeader")
mapDeprecatedOption("HTTPSecurityHeaders.CustomFrameOptionsValue", "HTTPHeaders.FrameOptions")
mapDeprecatedOption("CoverJpegQuality", "CoverArtQuality")
mapDeprecatedOption("SimilarSongsMatchThreshold", "Matcher.FuzzyThreshold")
mapDeprecatedOption("EnableTranscodingCancellation", "Transcoding.EnableCancellation")
for _, o := range deprecatedOptions {
if o.replacement != "" {
mapDeprecatedOption(o.name, o.replacement)
}
}
err := viper.Unmarshal(&Server, viper.DecodeHook(
mapstructure.ComposeDecodeHookFunc(
@ -403,6 +407,13 @@ func Load(noConfigDump bool) {
log.SetLogSourceLine(Server.DevLogSourceLine)
log.SetRedacting(Server.EnableLogRedacting)
// Log deprecated, removed and unknown options
for _, o := range deprecatedOptions {
logDeprecatedOptions(o.name, o.replacement)
}
logRemovedOptions(removedOptions...)
logUnknownOptions()
err = run.Sequentially(
validateScanSchedule,
validateBackupSchedule,
@ -461,21 +472,6 @@ func Load(noConfigDump bool) {
// Parse Deezer.Language into Languages slice (comma-separated, with fallback to DefaultInfoLanguage)
Server.Deezer.Languages = parseLanguages(Server.Deezer.Language)
// Deprecated options
logDeprecatedOptions("Scanner.GenreSeparators", "")
logDeprecatedOptions("Scanner.GroupAlbumReleases", "")
logDeprecatedOptions("DevEnableBufferedScrobble", "") // Deprecated: Buffered scrobbling is now always enabled and this option is ignored
logDeprecatedOptions("SearchFullString", "Search.FullString")
logDeprecatedOptions("ReverseProxyWhitelist", "ExtAuth.TrustedSources")
logDeprecatedOptions("ReverseProxyUserHeader", "ExtAuth.UserHeader")
logDeprecatedOptions("HTTPSecurityHeaders.CustomFrameOptionsValue", "HTTPHeaders.FrameOptions")
logDeprecatedOptions("CoverJpegQuality", "CoverArtQuality")
logDeprecatedOptions("SimilarSongsMatchThreshold", "Matcher.FuzzyThreshold")
logDeprecatedOptions("EnableTranscodingCancellation", "Transcoding.EnableCancellation")
// Removed options
logRemovedOptions("Spotify.ID", "Spotify.Secret")
// Validate other options
if Server.UICoverArtSize < 200 || Server.UICoverArtSize > 1200 {
newValue := max(200, min(1200, Server.UICoverArtSize))
@ -489,9 +485,26 @@ func Load(noConfigDump bool) {
}
}
// deprecatedOptions still work, but will be removed in a future release. An empty
// replacement means the option is now ignored.
var deprecatedOptions = []struct{ name, replacement string }{
{"Scanner.GenreSeparators", ""},
{"Scanner.GroupAlbumReleases", ""},
{"DevEnableBufferedScrobble", ""},
{"SearchFullString", "Search.FullString"},
{"ReverseProxyWhitelist", "ExtAuth.TrustedSources"},
{"ReverseProxyUserHeader", "ExtAuth.UserHeader"},
{"HTTPSecurityHeaders.CustomFrameOptionsValue", "HTTPHeaders.FrameOptions"},
{"CoverJpegQuality", "CoverArtQuality"},
{"SimilarSongsMatchThreshold", "Matcher.FuzzyThreshold"},
{"EnableTranscodingCancellation", "Transcoding.EnableCancellation"},
}
var removedOptions = []string{"Spotify.ID", "Spotify.Secret"}
func logDeprecatedOptions(oldName, newName string) {
envVar := "ND_" + strings.ToUpper(strings.ReplaceAll(oldName, ".", "_"))
newEnvVar := "ND_" + strings.ToUpper(strings.ReplaceAll(newName, ".", "_"))
envVar := envVarName(oldName)
newEnvVar := envVarName(newName)
logWarning := func(oldName, newName string) {
if newName != "" {
log.Warn(fmt.Sprintf("Option '%s' is deprecated and will be ignored in a future release. Please use the new '%s'", oldName, newName))
@ -511,7 +524,7 @@ func logDeprecatedOptions(oldName, newName string) {
// not available anymore
func logRemovedOptions(options ...string) {
for _, option := range options {
envVar := "ND_" + strings.ToUpper(strings.ReplaceAll(option, ".", "_"))
envVar := envVarName(option)
logWarning := func(option string) {
log.Warn(fmt.Sprintf("Option '%s' is not available anymore and will be ignored. Please remove it from your config", option))
}
@ -532,35 +545,193 @@ func remapEnvVarKeysFromConfig() {
continue
}
stripped := strings.TrimPrefix(key, "nd_")
canonicalKey := strings.ReplaceAll(stripped, "_", ".")
canonicalKey := ndKeyToCanonical(key)
displayNDKey := "ND_" + strings.ToUpper(stripped)
displayCanonical := toPascalCase(canonicalKey)
canonicalName := canonicalOptionName(canonicalKey)
if viper.InConfig(canonicalKey) {
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,
displayNDKey, cmp.Or(canonicalName, toPascalCase(canonicalKey)),
))
return
}
viper.Set(canonicalKey, viper.Get(key))
_, _ = fmt.Fprintf(os.Stderr, "WARNING: Config key '%s' uses environment variable naming. Use '%s' instead. "+
"The 'ND_' prefix is only needed for environment variables.\n",
displayNDKey, displayCanonical,
)
// Unknown keys get no advice here, logUnknownOptions reports them instead
if canonicalName != "" {
_, _ = fmt.Fprintf(os.Stderr, "WARNING: Config key '%s' uses environment variable naming. Use '%s' instead. "+
"The 'ND_' prefix is only needed for environment variables.\n",
displayNDKey, canonicalName,
)
}
}
}
// mapDeprecatedOption is used to provide backwards compatibility for deprecated options. It should be called after
// the config has been read by viper, but before unmarshalling it into the Config struct.
func mapDeprecatedOption(legacyName, newName string) {
if viper.IsSet(legacyName) {
// viper.Set outranks the config file, so an explicit replacement must win over the legacy value
if viper.IsSet(legacyName) && !explicitlySet(newName) {
viper.Set(newName, viper.Get(legacyName))
}
}
// explicitlySet reports whether the user provided the option, ignoring defaults,
// which viper.IsSet counts as set. The ND_ spelling is also accepted in the config
// file, and remapEnvVarKeysFromConfig has already moved it out of InConfig's reach.
func explicitlySet(name string) bool {
envVar := envVarName(name)
return viper.InConfig(name) || os.Getenv(envVar) != "" || viper.InConfig(strings.ToLower(envVar))
}
func envVarName(option string) string {
if option == "" {
return ""
}
return "ND_" + strings.ToUpper(strings.ReplaceAll(option, ".", "_"))
}
func logUnknownOptions() {
for _, key := range unknownConfigKeys() {
msg := fmt.Sprintf("Option '%s' is not recognized and will be ignored", key)
if matches := suggestOptions(key); len(matches) > 0 {
msg += fmt.Sprintf(". Did you mean '%s'?", strings.Join(matches, "' or '"))
}
log.Warn(msg)
}
}
// suggestOptions returns the known options sharing the last segment with key,
// catching options written outside their section.
func suggestOptions(key string) []string {
key = strings.ToLower(key)
leaf := leafKey(key)
canonical, _ := configKeys()
var matches []string
for known, name := range canonical {
// Removed options are known only so they get their own warning, never suggest them
if known != key && leafKey(known) == leaf && !slices.Contains(removedOptions, name) {
matches = append(matches, name)
}
}
slices.Sort(matches)
return matches
}
func leafKey(key string) string {
return key[strings.LastIndex(key, ".")+1:]
}
// unknownConfigKeys returns config file keys that don't match any known option, so
// typos and options written outside their section don't fail silently.
func unknownConfigKeys() []string {
// INI files keep the original [default] section alongside the merged one
skipDefault := strings.EqualFold(filepath.Ext(viper.ConfigFileUsed()), ".ini")
var unknown []string
for _, key := range viper.AllKeys() {
if !viper.InConfig(key) || canonicalOptionName(key) != "" {
continue
}
if skipDefault && strings.HasPrefix(key, "default.") {
continue
}
// Only ND_-prefixed keys that remapEnvVarKeysFromConfig could resolve are valid
if strings.HasPrefix(key, "nd_") && canonicalOptionName(ndKeyToCanonical(key)) != "" {
continue
}
unknown = append(unknown, key)
}
slices.Sort(unknown)
return asWrittenInConfigFile(unknown)
}
func ndKeyToCanonical(key string) string {
return strings.ReplaceAll(strings.TrimPrefix(key, "nd_"), "_", ".")
}
// canonicalOptionName returns the documented spelling of a known option key, or ""
// if it matches no option. Subkeys of free-form maps have no fixed spelling.
func canonicalOptionName(key string) string {
keys, prefixes := configKeys()
if name, ok := keys[key]; ok {
return name
}
if slices.ContainsFunc(prefixes, func(p string) bool { return strings.HasPrefix(key, p) }) {
return toPascalCase(key)
}
return ""
}
// asWrittenInConfigFile restores the casing the keys have in the config file, as
// viper lowercases every key it loads.
func asWrittenInConfigFile(keys []string) []string {
if len(keys) == 0 {
return nil
}
data, err := os.ReadFile(viper.ConfigFileUsed())
if err != nil {
return keys
}
casing := map[string]string{}
for _, match := range configFileKeyRx.FindAllStringSubmatch(string(data), -1) {
for segment := range strings.SplitSeq(match[1], ".") {
lower := strings.ToLower(segment)
casing[lower] = cmp.Or(casing[lower], segment)
}
}
return slice.Map(keys, func(key string) string {
segments := strings.Split(key, ".")
for i, s := range segments {
segments[i] = cmp.Or(casing[s], s)
}
return strings.Join(segments, ".")
})
}
// Matches keys and section headers in all supported config formats.
var configFileKeyRx = regexp.MustCompile(`(?m)^\s*\[?\s*"?([\w.]+)"?\s*[]=:]`)
// configKeys maps every accepted option name, lowercased, to its canonical spelling,
// plus the prefixes of free-form map options (Tags, DevLogLevels).
var configKeys = sync.OnceValues(func() (map[string]string, []string) {
keys := map[string]string{}
var prefixes []string
var collect func(t reflect.Type, prefix string)
collect = func(t reflect.Type, prefix string) {
for field := range t.Fields() {
// `conf:"-"` marks values computed during Load, not settable in the config
if !field.IsExported() || field.Tag.Get("conf") == "-" {
continue
}
name := prefix + field.Name
if field.Type.Kind() == reflect.Struct && !reflect.PointerTo(field.Type).Implements(textUnmarshalerType) {
collect(field.Type, name+".")
continue
}
lower := strings.ToLower(name)
keys[lower] = name
if field.Type.Kind() == reflect.Map {
prefixes = append(prefixes, lower+".")
}
}
}
collect(reflect.TypeFor[configOptions](), "")
for _, o := range deprecatedOptions {
keys[strings.ToLower(o.name)] = o.name
}
for _, o := range removedOptions {
keys[strings.ToLower(o)] = o
}
return keys, prefixes
})
var textUnmarshalerType = reflect.TypeFor[encoding.TextUnmarshaler]()
// parseIniFileConfiguration is used to parse the config file when it is in INI format. For INI files, it
// would require a nested structure, so instead we unmarshal it to a map and then merge the nested [default]
// section into the root level.

View File

@ -1,12 +1,14 @@
package conf_test
import (
"bytes"
"fmt"
"os"
"path/filepath"
"testing"
"github.com/navidrome/navidrome/conf"
"github.com/navidrome/navidrome/log"
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
"github.com/spf13/viper"
@ -178,6 +180,123 @@ var _ = Describe("Configuration", func() {
})
})
Describe("unknownConfigKeys", func() {
BeforeEach(func() {
viper.Reset()
conf.SetViperDefaults()
viper.SetDefault("datafolder", GinkgoT().TempDir())
viper.SetDefault("loglevel", "error")
conf.ResetConf()
})
It("reports misplaced and misspelled options, as spelled in the config file", func() {
conf.InitConfig(filepath.Join("testdata", "cfg_unknown_keys.toml"), false)
conf.Load(true)
Expect(conf.UnknownConfigKeys()).To(ConsistOf(
"ArtistSplitExceptions", "EnableDownlods", "Whatever.Foo",
))
})
DescribeTable("recovers the original casing in all supported formats",
func(file string) {
conf.InitConfig(filepath.Join("testdata", file), false)
conf.Load(true)
Expect(conf.UnknownConfigKeys()).To(ConsistOf("NotAnOption"))
},
Entry("TOML", "cfg_unknown_casing.toml"),
Entry("YAML", "cfg_unknown_casing.yaml"),
Entry("JSON", "cfg_unknown_casing.json"),
Entry("INI", "cfg_unknown_casing.ini"),
)
It("does not report valid, deprecated or free-form keys", func() {
conf.InitConfig(filepath.Join("testdata", "cfg.toml"), false)
conf.Load(true)
Expect(conf.UnknownConfigKeys()).To(BeEmpty())
})
It("does not report the [default] section of INI files", func() {
conf.InitConfig(filepath.Join("testdata", "cfg.ini"), false)
conf.Load(true)
Expect(conf.UnknownConfigKeys()).To(BeEmpty())
})
DescribeTable("SuggestOptions",
func(key string, expected []string) {
Expect(conf.SuggestOptions(key)).To(Equal(expected))
},
Entry("suggests the section of a misplaced option", "artistsplitexceptions",
[]string{"Scanner.ArtistSplitExceptions"}),
Entry("suggests the section of a misplaced nested option", "backup.fuzzythreshold",
[]string{"Matcher.FuzzyThreshold"}),
Entry("suggests every section defining the option", "schedule",
[]string{"Backup.Schedule", "Scanner.Schedule"}),
Entry("suggests nothing for a typo", "enabledownlods", nil),
)
It("does not report ND_-prefixed keys, as they are remapped", func() {
conf.InitConfig(filepath.Join("testdata", "cfg_nd_keys.toml"), false)
conf.Load(true)
Expect(conf.UnknownConfigKeys()).To(BeEmpty())
})
It("reports ND_-prefixed keys that remap to no known option", func() {
conf.InitConfig(filepath.Join("testdata", "cfg_nd_bogus.toml"), false)
conf.Load(true)
Expect(conf.UnknownConfigKeys()).To(ConsistOf("ND_TOTALLY_BOGUS_OPTION"))
Expect(conf.Server.Scanner.Schedule).To(Equal("@every 1h"))
})
It("migrates every deprecated option that has a replacement", func() {
conf.InitConfig(filepath.Join("testdata", "cfg_deprecated_search.toml"), false)
conf.Load(true)
Expect(conf.Server.Search.FullString).To(BeTrue())
Expect(conf.UnknownConfigKeys()).To(BeEmpty())
})
It("warns about each unrecognized option at startup", func() {
var logBuf bytes.Buffer
log.SetOutput(&logBuf)
DeferCleanup(func() { log.SetOutput(GinkgoWriter) })
conf.InitConfig(filepath.Join("testdata", "cfg_warning_output.toml"), false)
conf.Load(true)
Expect(logBuf.String()).To(ContainSubstring(
"Option 'ArtistSplitExceptions' is not recognized and will be ignored. " +
"Did you mean 'Scanner.ArtistSplitExceptions'?"))
Expect(logBuf.String()).To(ContainSubstring(
"Option 'EnableDownlods' is not recognized and will be ignored"))
Expect(logBuf.String()).ToNot(ContainSubstring("ArtistJoiner"))
})
Context("with runtime-computed and removed options in the config", func() {
BeforeEach(func() {
conf.InitConfig(filepath.Join("testdata", "cfg_runtime_fields.toml"), false)
conf.Load(true)
})
It("reports values computed during Load, which the config cannot set", func() {
Expect(conf.UnknownConfigKeys()).To(ContainElements("ConfigFile", "LastFM.Languages"))
})
It("never suggests a removed option", func() {
Expect(conf.SuggestOptions("id")).To(BeEmpty())
})
It("keeps an explicit replacement over the deprecated value", func() {
Expect(conf.Server.Search.FullString).To(BeFalse())
})
})
})
Describe("logFatal", func() {
var invalidPath string
BeforeEach(func() {

View File

@ -32,3 +32,7 @@ func SetLogFatal(f func(...any)) func() {
logFatal = f
return func() { logFatal = old }
}
var UnknownConfigKeys = unknownConfigKeys
var SuggestOptions = suggestOptions

View File

@ -0,0 +1,2 @@
MusicFolder = "/toml/music"
SearchFullString = true

3
conf/testdata/cfg_nd_bogus.toml vendored Normal file
View File

@ -0,0 +1,3 @@
MusicFolder = "/toml/music"
ND_TOTALLY_BOGUS_OPTION = true
ND_SCANNER_SCHEDULE = "@every 1h"

10
conf/testdata/cfg_runtime_fields.toml vendored Normal file
View File

@ -0,0 +1,10 @@
MusicFolder = "/toml/music"
SearchFullString = true
ConfigFile = "/somewhere/else"
ID = "oops"
[Search]
FullString = false
[LastFM]
Languages = ["pt"]

3
conf/testdata/cfg_unknown_casing.ini vendored Normal file
View File

@ -0,0 +1,3 @@
[default]
MusicFolder = /ini/music
NotAnOption = true

4
conf/testdata/cfg_unknown_casing.json vendored Normal file
View File

@ -0,0 +1,4 @@
{
"MusicFolder": "/json/music",
"NotAnOption": true
}

2
conf/testdata/cfg_unknown_casing.toml vendored Normal file
View File

@ -0,0 +1,2 @@
MusicFolder = "/toml/music"
NotAnOption = true

2
conf/testdata/cfg_unknown_casing.yaml vendored Normal file
View File

@ -0,0 +1,2 @@
MusicFolder: /yaml/music
NotAnOption: true

18
conf/testdata/cfg_unknown_keys.toml vendored Normal file
View File

@ -0,0 +1,18 @@
MusicFolder = "/toml/music"
# Valid option, but written at the root level instead of under Scanner
ArtistSplitExceptions = ["AC/DC", "Tyler, the creator"]
# Misspelled option
EnableDownlods = true
# Unknown section
[Whatever]
Foo = "bar"
# Valid options, must not be reported
[Scanner]
ArtistJoiner = " • "
[Tags.custom]
aliases = ["toml", "test"]

7
conf/testdata/cfg_warning_output.toml vendored Normal file
View File

@ -0,0 +1,7 @@
MusicFolder = "/toml/music"
LogLevel = "warn"
ArtistSplitExceptions = ["AC/DC"]
EnableDownlods = true
[Scanner]
ArtistJoiner = " • "