[confmap] fix bug of unmarshalling slice values (#8017)

fix the bug of unmarshalling slice values for package confmap

Fixes #4001

---------

Signed-off-by: Ziqi Zhao <[email protected]>

.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:

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
+	}
+}

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)
+			}
+		})
+	}
+}