diff --git a/.chloggen/config-unmarshel-slice-bug.yaml b/.chloggen/config-unmarshel-slice-bug.yaml new file mode 100755 index 00000000000..a96d52c75f1 --- /dev/null +++ b/.chloggen/config-unmarshel-slice-bug.yaml @@ -0,0 +1,16 @@ +# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' +change_type: bug_fix + +# The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver) +component: confmap + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: fix bugs of unmarshalling slice values + +# One or more tracking issues or pull requests related to the change +issues: [4001] + +# (Optional) One or more lines of additional information to render under the primary note. +# These lines will be padded with 2 spaces and then inserted directly into the document. +# Use pipe (|) for multiline entries. +subtext: diff --git a/confmap/confmap.go b/confmap/confmap.go index d1b4edfe1f2..254f0813433 100644 --- a/confmap/confmap.go +++ b/confmap/confmap.go @@ -157,6 +157,7 @@ func decodeConfig(m *Conf, result any, errorUnused bool) error { mapstructure.StringToTimeDurationHookFunc(), mapstructure.TextUnmarshallerHookFunc(), unmarshalerHookFunc(result), + zeroSliceHookFunc(), ), } decoder, err := mapstructure.NewDecoder(dc) @@ -336,3 +337,34 @@ type Marshaler interface { // The Conf will be empty and can be merged into. 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 overrided +// - 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 overrided by empty slics +// - 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 + } +} diff --git a/confmap/confmap_test.go b/confmap/confmap_test.go index fb5ee8d019a..211eca429aa 100644 --- a/confmap/confmap_test.go +++ b/confmap/confmap_test.go @@ -410,3 +410,88 @@ func TestUnmarshalerErr(t *testing.T) { assert.EqualError(t, cfgMap.Unmarshal(tc), expectErr) assert.Empty(t, tc.Err.Foo) } + +func TestZeroSliceHookFunc(t *testing.T) { + type structWithSlices struct { + Strings []string `mapstructure:"strings"` + } + + tests := []struct { + name string + cfg map[string]any + provided any + expected any + }{ + { + name: "overridden by slice", + cfg: map[string]any{ + "strings": []string{"111"}, + }, + provided: &structWithSlices{ + Strings: []string{"xxx", "yyyy", "zzzz"}, + }, + expected: &structWithSlices{ + Strings: []string{"111"}, + }, + }, + { + name: "overridden by a bigger slice", + cfg: map[string]any{ + "strings": []string{"111", "222", "333"}, + }, + provided: &structWithSlices{ + Strings: []string{"xxx", "yyyy"}, + }, + expected: &structWithSlices{ + Strings: []string{"111", "222", "333"}, + }, + }, + { + name: "overridden by an empty slice", + cfg: map[string]any{ + "strings": []string{}, + }, + provided: &structWithSlices{ + Strings: []string{"xxx", "yyyy"}, + }, + expected: &structWithSlices{ + Strings: []string{}, + }, + }, + { + name: "not overridden by nil", + cfg: map[string]any{ + "strings": nil, + }, + provided: &structWithSlices{ + Strings: []string{"xxx", "yyyy"}, + }, + expected: &structWithSlices{ + Strings: []string{"xxx", "yyyy"}, + }, + }, + { + name: "not overridden by missing value", + cfg: map[string]any{}, + provided: &structWithSlices{ + Strings: []string{"xxx", "yyyy"}, + }, + expected: &structWithSlices{ + Strings: []string{"xxx", "yyyy"}, + }, + }, + } + + for _, tt := range tests { + tt := tt + + t.Run(tt.name, func(t *testing.T) { + cfg := NewFromStringMap(tt.cfg) + + err := cfg.Unmarshal(tt.provided) + if assert.NoError(t, err) { + assert.Equal(t, tt.expected, tt.provided) + } + }) + } +}