Simplify unmarshal logic by adding more supported hooks (#4237)

* Simplify unmarshal logic by adding more supported hooks

* Add hook that supports "String -> encoding.TextUnmarshaler", e.g. zapcore.Level no longer need special unmarshaling
* Add hook that supports "String -> ComponentID"
* Add a special hook for map[string]interface{} -> map[ComponentID]interface{} to determine duplicates after space trimming, not sure if this error needs this special treatment.

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

* Fix review comments

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

* Improve error messages for unmarshaling errors of the ComponentID

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

Author: bogdandrutu

config/config.go

@@ -159,7 +159,7 @@ type Service struct {
 
 // ServiceTelemetry defines the configurable settings for service telemetry.
 type ServiceTelemetry struct {
-	Logs ServiceTelemetryLogs
+	Logs ServiceTelemetryLogs `mapstructure:"logs"`
 }
 
 func (srvT *ServiceTelemetry) validate() error {
@@ -171,15 +171,15 @@ func (srvT *ServiceTelemetry) validate() error {
 // the collector uses mapstructure and not yaml tags.
 type ServiceTelemetryLogs struct {
 	// Level is the minimum enabled logging level.
-	Level zapcore.Level
+	Level zapcore.Level `mapstructure:"level"`
 
 	// Development puts the logger in development mode, which changes the
 	// behavior of DPanicLevel and takes stacktraces more liberally.
-	Development bool
+	Development bool `mapstructure:"development"`
 
 	// Encoding sets the logger's encoding.
 	// Valid values are "json" and "console".
-	Encoding string
+	Encoding string `mapstructure:"encoding"`
 }
 
 func (srvTL *ServiceTelemetryLogs) validate() error {
@@ -226,9 +226,9 @@ const (
 type Pipeline struct {
 	Name       string
 	InputType  DataType
-	Receivers  []ComponentID
-	Processors []ComponentID
-	Exporters  []ComponentID
+	Receivers  []ComponentID `mapstructure:"receivers"`
+	Processors []ComponentID `mapstructure:"processors"`
+	Exporters  []ComponentID `mapstructure:"exporters"`
 }
 
 // Pipelines is a map of names to Pipelines.

config/configgrpc/configgrpc_test.go

@@ -209,7 +209,7 @@ func TestGRPCClientSettingsError(t *testing.T) {
 			},
 		},
 		{
-			err: "idStr must have non empty type",
+			err: "id must not be empty",
 			settings: GRPCClientSettings{
 				Endpoint: "localhost:1234",
 				Auth:     &configauth.Authentication{},

config/confighttp/confighttp_test.go

@@ -135,7 +135,7 @@ func TestHTTPClientSettingsError(t *testing.T) {
 			},
 		},
 		{
-			err: "idStr must have non empty type",
+			err: "id must not be empty",
 			settings: HTTPClientSettings{
 				Endpoint: "https://localhost:1234/v1/traces",
 				Auth:     &configauth.Authentication{AuthenticatorName: ""},

config/configmap.go

@@ -96,8 +96,8 @@ func (l *Map) Unmarshal(rawVal interface{}) error {
 }
 
 // UnmarshalExact unmarshalls the config into a struct, erroring if a field is nonexistent.
-func (l *Map) UnmarshalExact(intoCfg interface{}) error {
-	dc := decoderConfig(intoCfg)
+func (l *Map) UnmarshalExact(rawVal interface{}) error {
+	dc := decoderConfig(rawVal)
 	dc.ErrorUnused = true
 	decoder, err := mapstructure.NewDecoder(dc)
 	if err != nil {
@@ -164,13 +164,23 @@ func decoderConfig(result interface{}) *mapstructure.DecoderConfig {
 		TagName:          "mapstructure",
 		WeaklyTypedInput: true,
 		DecodeHook: mapstructure.ComposeDecodeHookFunc(
-			expandNilStructPointers(),
-			mapstructure.StringToTimeDurationHookFunc(),
-			mapstructure.StringToSliceHookFunc(","),
+			expandNilStructPointersFunc,
+			stringToSliceHookFunc,
+			mapStringToMapComponentIDHookFunc,
+			stringToTimeDurationHookFunc,
+			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
@@ -185,26 +195,51 @@ func decoderConfig(result interface{}) *mapstructure.DecoderConfig {
 //
 // we want an unmarshaled Config to be equivalent to
 // Config{Thing: &SomeStruct{}} instead of Config{Thing: nil}
-func expandNilStructPointers() mapstructure.DecodeHookFunc {
-	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)
-					}
+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)
 				}
 			}
 		}
-		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,
+// 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
+	}
+
+	if t.Kind() != reflect.Map || t.Key() != componentIDType {
+		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 := m[id]; ok {
+			return nil, fmt.Errorf("duplicate name %q after trimming spaces %v", k, id)
+		}
+		m[id] = v
+	}
+	return m, nil
 }