Extend unmarshal hook to check map key string to any TextUnmarshaler …

…not only ComponentID (open-telemetry#5244)

Signed-off-by: Bogdan Drutu <[email protected]>

CHANGELOG.md

@@ -26,6 +26,8 @@
 
 ### 💡 Enhancements 💡
 
+- Extend config.Map.Unmarshal hook to check map key string to any TextUnmarshaler not only ComponentID (#5244)
+
 ### 🧰 Bug fixes 🧰
 - Fix translation from otlp.Request to pdata representation, changes to the returned pdata not all reflected to the otlp.Request (#5197)
 - `exporterhelper` now properly consumes any remaining items on stop (#5203)

config/configmap.go

@@ -16,6 +16,7 @@ package config // import "go.opentelemetry.io/collector/config"
 
 import (
 	"context"
+	"encoding"
 	"fmt"
 	"reflect"
 
@@ -139,23 +140,15 @@ func decoderConfig(result interface{}) *mapstructure.DecoderConfig {
 		TagName:          "mapstructure",
 		WeaklyTypedInput: true,
 		DecodeHook: mapstructure.ComposeDecodeHookFunc(
-			expandNilStructPointersFunc,
-			stringToSliceHookFunc,
-			mapStringToMapComponentIDHookFunc,
-			stringToTimeDurationHookFunc,
-			textUnmarshallerHookFunc,
+			expandNilStructPointersHookFunc(),
+			mapstructure.StringToSliceHookFunc(","),
+			mapKeyStringToMapKeyTextUnmarshalerHookFunc(),
+			mapstructure.StringToTimeDurationHookFunc(),
+			mapstructure.TextUnmarshallerHookFunc(),
 		),
 	}
 }
 
-var (
-	stringToSliceHookFunc        = mapstructure.StringToSliceHookFunc(",")
-	stringToTimeDurationHookFunc = mapstructure.StringToTimeDurationHookFunc()
-	textUnmarshallerHookFunc     = mapstructure.TextUnmarshallerHookFunc()
-
-	componentIDType = reflect.TypeOf(NewComponentID("foo"))
-)
-
 // In cases where a config has a mapping of something to a struct pointers
 // we want nil values to resolve to a pointer to the zero value of the
 // underlying struct just as we want nil values of a mapping of something
@@ -170,51 +163,62 @@ var (
 //
 // we want an unmarshaled Config to be equivalent to
 // Config{Thing: &SomeStruct{}} instead of Config{Thing: nil}
-var expandNilStructPointersFunc = func(from reflect.Value, to reflect.Value) (interface{}, error) {
-	// ensure we are dealing with map to map comparison
-	if from.Kind() == reflect.Map && to.Kind() == reflect.Map {
-		toElem := to.Type().Elem()
-		// ensure that map values are pointers to a struct
-		// (that may be nil and require manual setting w/ zero value)
-		if toElem.Kind() == reflect.Ptr && toElem.Elem().Kind() == reflect.Struct {
-			fromRange := from.MapRange()
-			for fromRange.Next() {
-				fromKey := fromRange.Key()
-				fromValue := fromRange.Value()
-				// ensure that we've run into a nil pointer instance
-				if fromValue.IsNil() {
-					newFromValue := reflect.New(toElem.Elem())
-					from.SetMapIndex(fromKey, newFromValue)
+func expandNilStructPointersHookFunc() mapstructure.DecodeHookFuncValue {
+	return func(from reflect.Value, to reflect.Value) (interface{}, error) {
+		// ensure we are dealing with map to map comparison
+		if from.Kind() == reflect.Map && to.Kind() == reflect.Map {
+			toElem := to.Type().Elem()
+			// ensure that map values are pointers to a struct
+			// (that may be nil and require manual setting w/ zero value)
+			if toElem.Kind() == reflect.Ptr && toElem.Elem().Kind() == reflect.Struct {
+				fromRange := from.MapRange()
+				for fromRange.Next() {
+					fromKey := fromRange.Key()
+					fromValue := fromRange.Value()
+					// ensure that we've run into a nil pointer instance
+					if fromValue.IsNil() {
+						newFromValue := reflect.New(toElem.Elem())
+						from.SetMapIndex(fromKey, newFromValue)
+					}
 				}
 			}
 		}
+		return from.Interface(), nil
 	}
-	return from.Interface(), nil
 }
 
-// mapStringToMapComponentIDHookFunc returns a DecodeHookFunc that converts a map[string]interface{} to
-// map[ComponentID]interface{}.
-// This is needed in combination since the ComponentID.UnmarshalText may produce equal IDs for different strings,
+// mapKeyStringToMapKeyTextUnmarshalerHookFunc returns a DecodeHookFuncType that checks that a conversion from
+// map[string]interface{} to map[encoding.TextUnmarshaler]interface{} does not overwrite keys,
+// when UnmarshalText produces equal elements from different strings (e.g. trims whitespaces).
+//
+// This is needed in combination with ComponentID, which may produce equal IDs for different strings,
 // and an error needs to be returned in that case, otherwise the last equivalent ID overwrites the previous one.
-var mapStringToMapComponentIDHookFunc = func(f reflect.Type, t reflect.Type, data interface{}) (interface{}, error) {
-	if f.Kind() != reflect.Map || f.Key().Kind() != reflect.String {
-		return data, nil
-	}
+func mapKeyStringToMapKeyTextUnmarshalerHookFunc() mapstructure.DecodeHookFuncType {
+	return func(f reflect.Type, t reflect.Type, data interface{}) (interface{}, error) {
+		if f.Kind() != reflect.Map || f.Key().Kind() != reflect.String {
+			return data, nil
+		}
 
-	if t.Kind() != reflect.Map || t.Key() != componentIDType {
-		return data, nil
-	}
+		if t.Kind() != reflect.Map {
+			return data, nil
+		}
 
-	m := make(map[ComponentID]interface{})
-	for k, v := range data.(map[string]interface{}) {
-		id, err := NewComponentIDFromString(k)
-		if err != nil {
-			return nil, err
+		if _, ok := reflect.New(t.Key()).Interface().(encoding.TextUnmarshaler); !ok {
+			return data, nil
 		}
-		if _, ok := m[id]; ok {
-			return nil, fmt.Errorf("duplicate name %q after trimming spaces %v", k, id)
+
+		m := reflect.MakeMap(reflect.MapOf(t.Key(), reflect.TypeOf(true)))
+		for k := range data.(map[string]interface{}) {
+			tKey := reflect.New(t.Key())
+			if err := tKey.Interface().(encoding.TextUnmarshaler).UnmarshalText([]byte(k)); err != nil {
+				return nil, err
+			}
+
+			if m.MapIndex(reflect.Indirect(tKey)).IsValid() {
+				return nil, fmt.Errorf("duplicate name %q after unmarshaling %v", k, tKey)
+			}
+			m.SetMapIndex(reflect.Indirect(tKey), reflect.ValueOf(true))
 		}
-		m[id] = v
+		return data, nil
 	}
-	return m, nil
 }

config/configmap_test.go

@@ -15,9 +15,11 @@
 package config
 
 import (
+	"errors"
 	"fmt"
 	"io/ioutil"
 	"path/filepath"
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -124,18 +126,18 @@ func newMapFromFile(fileName string) (*Map, error) {
 	return NewMapFromStringMap(data), nil
 }
 
-func TestExpandNilStructPointersFunc(t *testing.T) {
+func TestExpandNilStructPointersHookFunc(t *testing.T) {
 	stringMap := map[string]interface{}{
 		"boolean": nil,
 		"struct":  nil,
 		"map_struct": map[string]interface{}{
 			"struct": nil,
 		},
 	}
-	parser := NewMapFromStringMap(stringMap)
+	cfgMap := NewMapFromStringMap(stringMap)
 	cfg := &TestConfig{}
 	assert.Nil(t, cfg.Struct)
-	assert.NoError(t, parser.UnmarshalExact(cfg))
+	assert.NoError(t, cfgMap.UnmarshalExact(cfg))
 	assert.Nil(t, cfg.Boolean)
 	// assert.False(t, *cfg.Boolean)
 	assert.Nil(t, cfg.Struct)
@@ -144,15 +146,15 @@ func TestExpandNilStructPointersFunc(t *testing.T) {
 	assert.Equal(t, &Struct{}, cfg.MapStruct["struct"])
 }
 
-func TestExpandNilStructPointersFunc_DefaultNotNilConfigNil(t *testing.T) {
+func TestExpandNilStructPointersHookFuncDefaultNotNilConfigNil(t *testing.T) {
 	stringMap := map[string]interface{}{
 		"boolean": nil,
 		"struct":  nil,
 		"map_struct": map[string]interface{}{
 			"struct": nil,
 		},
 	}
-	parser := NewMapFromStringMap(stringMap)
+	cfgMap := NewMapFromStringMap(stringMap)
 	varBool := true
 	s1 := &Struct{Name: "s1"}
 	s2 := &Struct{Name: "s2"}
@@ -161,7 +163,7 @@ func TestExpandNilStructPointersFunc_DefaultNotNilConfigNil(t *testing.T) {
 		Struct:    s1,
 		MapStruct: map[string]*Struct{"struct": s2},
 	}
-	assert.NoError(t, parser.UnmarshalExact(cfg))
+	assert.NoError(t, cfgMap.UnmarshalExact(cfg))
 	assert.NotNil(t, cfg.Boolean)
 	assert.True(t, *cfg.Boolean)
 	assert.NotNil(t, cfg.Struct)
@@ -180,3 +182,57 @@ type TestConfig struct {
 type Struct struct {
 	Name string
 }
+
+type TestID string
+
+func (tID *TestID) UnmarshalText(text []byte) error {
+	*tID = TestID(strings.TrimSuffix(string(text), "_"))
+	if *tID == "error" {
+		return errors.New("parsing error")
+	}
+	return nil
+}
+
+type TestIDConfig struct {
+	Boolean bool              `mapstructure:"bool"`
+	Map     map[TestID]string `mapstructure:"map"`
+}
+
+func TestMapKeyStringToMapKeyTextUnmarshalerHookFunc(t *testing.T) {
+	stringMap := map[string]interface{}{
+		"bool": true,
+		"map": map[string]interface{}{
+			"string": "this is a string",
+		},
+	}
+	cfgMap := NewMapFromStringMap(stringMap)
+	cfg := &TestIDConfig{}
+	assert.NoError(t, cfgMap.UnmarshalExact(cfg))
+	assert.True(t, cfg.Boolean)
+	assert.Equal(t, map[TestID]string{"string": "this is a string"}, cfg.Map)
+}
+
+func TestMapKeyStringToMapKeyTextUnmarshalerHookFuncDuplicateID(t *testing.T) {
+	stringMap := map[string]interface{}{
+		"bool": true,
+		"map": map[string]interface{}{
+			"string":  "this is a string",
+			"string_": "this is another string",
+		},
+	}
+	cfgMap := NewMapFromStringMap(stringMap)
+	cfg := &TestIDConfig{}
+	assert.Error(t, cfgMap.UnmarshalExact(cfg))
+}
+
+func TestMapKeyStringToMapKeyTextUnmarshalerHookFuncErrorUnmarshal(t *testing.T) {
+	stringMap := map[string]interface{}{
+		"bool": true,
+		"map": map[string]interface{}{
+			"error": "this is a string",
+		},
+	}
+	cfgMap := NewMapFromStringMap(stringMap)
+	cfg := &TestIDConfig{}
+	assert.Error(t, cfgMap.UnmarshalExact(cfg))
+}