From ea8937a9ee17d3091f7838ec6c0b7ed62ff5342f Mon Sep 17 00:00:00 2001 From: Bogdan Drutu Date: Fri, 22 Apr 2022 08:45:23 -0700 Subject: [PATCH] Extend unmarshal hook to check map key string to any TextUnmarshaler not only ComponentID (#5244) Signed-off-by: Bogdan Drutu --- CHANGELOG.md | 2 + config/configmap.go | 100 ++++++++++++++++++++------------------- config/configmap_test.go | 68 +++++++++++++++++++++++--- 3 files changed, 116 insertions(+), 54 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c4bea2f98806..5b5f1ea89dc8 100644 --- a/CHANGELOG.md +++ b/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) diff --git a/config/configmap.go b/config/configmap.go index 2fcef916836f..2ff60555786f 100644 --- a/config/configmap.go +++ b/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 } diff --git a/config/configmap_test.go b/config/configmap_test.go index dc54cef6b19c..a487c0e7ca83 100644 --- a/config/configmap_test.go +++ b/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,7 +126,7 @@ 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, @@ -132,10 +134,10 @@ func TestExpandNilStructPointersFunc(t *testing.T) { "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,7 +146,7 @@ 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, @@ -152,7 +154,7 @@ func TestExpandNilStructPointersFunc_DefaultNotNilConfigNil(t *testing.T) { "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)) +}