diff --git a/confmap/confmap.go b/confmap/confmap.go index a7a99d10352..2e757024a34 100644 --- a/confmap/confmap.go +++ b/confmap/confmap.go @@ -133,6 +133,10 @@ func sanitizeExpanded(a any, useOriginal bool) any { return c case []any: var newSlice []any + if m == nil { + return newSlice + } + newSlice = []any{} for _, e := range m { newSlice = append(newSlice, sanitizeExpanded(e, useOriginal)) } @@ -219,7 +223,6 @@ func decodeConfig(m *Conf, result any, errorUnused bool, skipTopLevelUnmarshaler // after the main unmarshaler hook is called, // we unmarshal the embedded structs if present to merge with the result: unmarshalerEmbeddedStructsHookFunc(), - zeroSliceHookFunc(), ), } decoder, err := mapstructure.NewDecoder(dc) @@ -513,37 +516,6 @@ type Marshaler interface { Marshal(component *Conf) error } -// This hook is used to solve the issue: https://github.com/open-telemetry/opentelemetry-collector/issues/4001 -// We adopt the suggestion provided in this issue: https://github.com/mitchellh/mapstructure/issues/74#issuecomment-279886492 -// We should empty every slice before unmarshalling unless user provided slice is nil. -// Assume that we had a struct with a field of type slice called `keys`, which has default values of ["a", "b"] -// -// type Config struct { -// Keys []string `mapstructure:"keys"` -// } -// -// The configuration provided by users may have following cases -// 1. configuration have `keys` field and have a non-nil values for this key, the output should be overridden -// - for example, input is {"keys", ["c"]}, then output is Config{ Keys: ["c"]} -// -// 2. configuration have `keys` field and have an empty slice for this key, the output should be overridden by empty slices -// - for example, input is {"keys", []}, then output is Config{ Keys: []} -// -// 3. configuration have `keys` field and have nil value for this key, the output should be default config -// - for example, input is {"keys": nil}, then output is Config{ Keys: ["a", "b"]} -// -// 4. configuration have no `keys` field specified, the output should be default config -// - for example, input is {}, then output is Config{ Keys: ["a", "b"]} -func zeroSliceHookFunc() mapstructure.DecodeHookFuncValue { - return func(from reflect.Value, to reflect.Value) (interface{}, error) { - if to.CanSet() && to.Kind() == reflect.Slice && from.Kind() == reflect.Slice { - to.Set(reflect.MakeSlice(to.Type(), from.Len(), from.Cap())) - } - - return from.Interface(), nil - } -} - type moduleFactory[T any, S any] interface { Create(s S) T } diff --git a/confmap/confmap_test.go b/confmap/confmap_test.go index 7055b683fa7..06897450902 100644 --- a/confmap/confmap_test.go +++ b/confmap/confmap_test.go @@ -691,6 +691,50 @@ func TestZeroSliceHookFunc(t *testing.T) { } } +func TestNilValuesUnchanged(t *testing.T) { + type structWithSlices struct { + Strings []string `mapstructure:"strings"` + } + + slicesStruct := &structWithSlices{} + + nilCfg := map[string]any{ + "strings": []any(nil), + } + nilConf := NewFromStringMap(nilCfg) + err := nilConf.Unmarshal(slicesStruct) + require.NoError(t, err) + + confFromStruct := New() + err = confFromStruct.Marshal(slicesStruct) + require.NoError(t, err) + + require.Equal(t, nilCfg, nilConf.ToStringMap()) + require.EqualValues(t, nilConf.ToStringMap(), confFromStruct.ToStringMap()) +} + +func TestEmptySliceUnchanged(t *testing.T) { + type structWithSlices struct { + Strings []string `mapstructure:"strings"` + } + + slicesStruct := &structWithSlices{} + + nilCfg := map[string]any{ + "strings": []any{}, + } + nilConf := NewFromStringMap(nilCfg) + err := nilConf.Unmarshal(slicesStruct) + require.NoError(t, err) + + confFromStruct := New() + err = confFromStruct.Marshal(slicesStruct) + require.NoError(t, err) + + require.Equal(t, nilCfg, nilConf.ToStringMap()) + require.EqualValues(t, nilConf.ToStringMap(), confFromStruct.ToStringMap()) +} + type C struct { Modifiers []string `mapstructure:"modifiers"` } diff --git a/confmap/confmaptest/configtest_test.go b/confmap/confmaptest/configtest_test.go index cf7717071cf..33c98d2f665 100644 --- a/confmap/confmaptest/configtest_test.go +++ b/confmap/confmaptest/configtest_test.go @@ -30,11 +30,16 @@ func TestLoadConf(t *testing.T) { assert.Equal(t, map[string]any{"floating": 3.14}, cfg.ToStringMap()) } -func TestToStringMapSanitizeEmptySlice(t *testing.T) { +func TestToStringMapSanitizeNil(t *testing.T) { + cfg, err := LoadConf(filepath.Join("testdata", "nil.yaml")) + require.NoError(t, err) + assert.Equal(t, map[string]any{"slice": nil}, cfg.ToStringMap()) +} + +func TestToStringMapEmptySlice(t *testing.T) { cfg, err := LoadConf(filepath.Join("testdata", "empty-slice.yaml")) require.NoError(t, err) - var nilSlice []interface{} - assert.Equal(t, map[string]any{"slice": nilSlice}, cfg.ToStringMap()) + assert.Equal(t, map[string]any{"slice": []any{}}, cfg.ToStringMap()) } func TestValidateProviderScheme(t *testing.T) { diff --git a/confmap/confmaptest/testdata/empty-slice.yaml b/confmap/confmaptest/testdata/empty-slice.yaml index ec9c7c062ae..797254b64a9 100644 --- a/confmap/confmaptest/testdata/empty-slice.yaml +++ b/confmap/confmaptest/testdata/empty-slice.yaml @@ -1 +1 @@ -slice: [] # empty slices are sanitized to nil in ToStringMap +slice: [] diff --git a/confmap/confmaptest/testdata/nil.yaml b/confmap/confmaptest/testdata/nil.yaml new file mode 100644 index 00000000000..d24c2453dc0 --- /dev/null +++ b/confmap/confmaptest/testdata/nil.yaml @@ -0,0 +1 @@ +slice: diff --git a/confmap/internal/mapstructure/encoder.go b/confmap/internal/mapstructure/encoder.go index 994ad038fd3..7e4e6baa961 100644 --- a/confmap/internal/mapstructure/encoder.go +++ b/confmap/internal/mapstructure/encoder.go @@ -143,6 +143,9 @@ func (e *Encoder) encodeSlice(value reflect.Value) (any, error) { Kind: value.Kind(), } } + if value.IsNil() { + return []any(nil), nil + } result := make([]any, value.Len()) for i := 0; i < value.Len(); i++ { var err error