From 55a06db0b32783539a2c7d025a582742323c2eaf Mon Sep 17 00:00:00 2001 From: Calabe Davis Date: Fri, 12 Jun 2026 07:40:41 -0700 Subject: [PATCH] Fix module config overlay clobbering operator overrides Two related bugs in the module config write/merge path: 1. AddOverlayOverrides applied only a module's NEW overlay keys via OverlayOverrides. Because Config.Modules is a plain map, unmarshaling that partial Modules block replaced the inner Modules[] map wholesale, silently discarding every operator-set value for that module (config-overrides.yaml itself was untouched, so it re-broke on every boot). Triggered whenever a module upgrade ships a config key the operator's partial Modules block lacks. Now the full (flattened) override set is overlaid, matching SetVal's proven merge behavior, so operator values survive while new keys still get their overlay defaults. 2. PluginConfig.Set discarded the error from configs.SetVal, so failed module config writes (invalid property name, file write errors) gave no signal anywhere. The error is now logged with the plugin and key name. (Returning the error would be stronger but changes the method signature, breaking out-of-tree modules; left as a follow-up option.) Adds a regression test covering the partial-Modules-block boot sequence. Co-Authored-By: Claude Fable 5 --- internal/configs/configs.go | 10 ++++- internal/configs/overrides_test.go | 64 ++++++++++++++++++++++++++++++ internal/plugins/pluginconfig.go | 5 ++- 3 files changed, 77 insertions(+), 2 deletions(-) diff --git a/internal/configs/configs.go b/internal/configs/configs.go index 9e2723bba..a89b22170 100644 --- a/internal/configs/configs.go +++ b/internal/configs/configs.go @@ -82,7 +82,15 @@ func AddOverlayOverrides(dotMap map[string]any) error { if len(newKeys) == 0 { return nil } - return configData.OverlayOverrides(newKeys) + + // Overlay the full override set rather than just the new keys. Overlaying + // only the new keys would unmarshal a partial Modules block into the live + // config, replacing the inner Modules[] map wholesale and discarding + // every operator-supplied value for that module. Flatten first: at this + // point `overrides` is mixed-shape (nested maps loaded from + // config-overrides.yaml plus the flat dotted keys added above), and + // OverlayOverrides re-nests it internally. + return configData.OverlayOverrides(Flatten(overrides)) } // OverlayDotMap overlays values from a dot-syntax map onto the Config. diff --git a/internal/configs/overrides_test.go b/internal/configs/overrides_test.go index c5b50adb4..98df462af 100644 --- a/internal/configs/overrides_test.go +++ b/internal/configs/overrides_test.go @@ -3,6 +3,7 @@ package configs import ( "testing" + "github.com/stretchr/testify/require" "gopkg.in/yaml.v2" ) @@ -95,3 +96,66 @@ func TestOverlayDotMapMultipleFields(t *testing.T) { t.Errorf("Expected SomeField to be 'updated', got '%s'", cfg.Statistics.SomeField) } } + +// TestAddOverlayOverridesPreservesOperatorOverrides reproduces the boot +// sequence for a world whose config-overrides.yaml contains a partial +// Modules block: the operator has set some (but not all) of a module's keys, +// and the module's data-overlay config ships a superset of those keys. +// Applying the overlay must fill in the missing defaults WITHOUT clobbering +// the operator-supplied values or other modules' blocks. +func TestAddOverlayOverridesPreservesOperatorOverrides(t *testing.T) { + + // Snapshot and restore package globals so this test is hermetic. + origConfigData := configData + origOverrides := overrides + origKeyLookups := keyLookups + origTypeLookups := typeLookups + t.Cleanup(func() { + configData = origConfigData + overrides = origOverrides + keyLookups = origKeyLookups + typeLookups = origTypeLookups + }) + + configData = Config{} + keyLookups = map[string]string{} + typeLookups = map[string]string{} + + // The operator's config-overrides.yaml: a partial Modules block for + // "weather" (missing NewSetting), plus an unrelated module's block. + operatorYAML := []byte(` +Modules: + weather: + Enabled: true + CycleSeconds: 120 + othermod: + Setting: keepme +`) + loadedOverrides := map[string]any{} + require.NoError(t, yaml.Unmarshal(operatorYAML, &loadedOverrides)) + overrides = loadedOverrides + + // ReloadConfig applies operator overrides onto the live config at boot. + require.NoError(t, configData.OverlayOverrides(overrides)) + + // The module loads and registers its data-overlay config, a superset of + // the operator's keys. This mirrors how internal/plugins builds the map. + err := AddOverlayOverrides(map[string]any{ + `Modules.weather.Enabled`: false, // module default; operator set true + `Modules.weather.CycleSeconds`: 60, // module default; operator set 120 + `Modules.weather.NewSetting`: `overlayval`, // new key, absent from operator overrides + }) + require.NoError(t, err) + + flat := Flatten(map[string]any(configData.Modules)) + + // (a) Operator-supplied values must survive in the live config. + require.Equal(t, true, flat[`weather.Enabled`], `operator override Modules.weather.Enabled was clobbered by the module overlay`) + require.Equal(t, 120, flat[`weather.CycleSeconds`], `operator override Modules.weather.CycleSeconds was clobbered by the module overlay`) + + // (b) Keys absent from the operator overrides get the overlay defaults. + require.Equal(t, `overlayval`, flat[`weather.NewSetting`]) + + // (c) Other modules' blocks are untouched. + require.Equal(t, `keepme`, flat[`othermod.Setting`]) +} diff --git a/internal/plugins/pluginconfig.go b/internal/plugins/pluginconfig.go index 515d2d583..ef6611385 100644 --- a/internal/plugins/pluginconfig.go +++ b/internal/plugins/pluginconfig.go @@ -4,6 +4,7 @@ import ( "fmt" "github.com/GoMudEngine/GoMud/internal/configs" + "github.com/GoMudEngine/GoMud/internal/mudlog" ) type PluginConfig struct { @@ -11,7 +12,9 @@ type PluginConfig struct { } func (p *PluginConfig) Set(name string, val any) { - configs.SetVal(fmt.Sprintf(`Modules.%s.%s`, p.pluginName, name), fmt.Sprintf(`%v`, val)) + if err := configs.SetVal(fmt.Sprintf(`Modules.%s.%s`, p.pluginName, name), fmt.Sprintf(`%v`, val)); err != nil { + mudlog.Error(`PluginConfig.Set`, `plugin`, p.pluginName, `key`, name, `error`, err) + } } func (p *PluginConfig) Get(name string) any {