[component] Print path to error in ValidateConfig

.chloggen/validateconfig-print-path.yaml

@@ -0,0 +1,25 @@
+# Use this changelog template to create an entry for release notes.
+
+# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
+change_type: enhancement
+
+# The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver)
+component: component
+
+# A brief description of the change.  Surround your text with quotes ("") if it needs to start with a backtick (`).
+note: Show path to invalid config in errors returned from `component.ValidateConfig`
+
+# One or more tracking issues or pull requests related to the change
+issues: [12108]
+
+# (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:
+
+# Optional: The change log or logs in which this entry should be included.
+# e.g. '[user]' or '[user, api]'
+# Include 'user' if the change is relevant to end users.
+# Include 'api' if there is a change to a library API.
+# Default: '[user]'
+change_logs: [api]

component/config.go

@@ -4,9 +4,11 @@
 package component // import "go.opentelemetry.io/collector/component"
 
 import (
+	"errors"
+	"fmt"
 	"reflect"
-
-	"go.uber.org/multierr"
+	"strconv"
+	"strings"
 )
 
 // Config defines the configuration for a component.Component.
@@ -31,46 +33,131 @@ type ConfigValidator interface {
 // ValidateConfig validates a config, by doing this:
 //   - Call Validate on the config itself if the config implements ConfigValidator.
 func ValidateConfig(cfg Config) error {
-	return validate(reflect.ValueOf(cfg))
+	var err error
+	errs := validate(reflect.ValueOf(cfg))
+
+	for _, suberr := range errs {
+		if suberr.err != nil {
+			if suberr.path != "" {
+				err = errors.Join(err, fmt.Errorf("%s: %w", suberr.path, suberr.err))
+			} else {
+				err = errors.Join(err, suberr.err)
+			}
+		}
+	}
+
+	return err
+}
+
+type pathError struct {
+	err  error
+	path string
 }
 
-func validate(v reflect.Value) error {
+func validate(v reflect.Value) []pathError {
+	errs := []pathError{}
 	// Validate the value itself.
 	switch v.Kind() {
 	case reflect.Invalid:
 		return nil
 	case reflect.Ptr, reflect.Interface:
 		return validate(v.Elem())
 	case reflect.Struct:
-		var errs error
-		errs = multierr.Append(errs, callValidateIfPossible(v))
+		errs = append(errs, pathError{err: callValidateIfPossible(v), path: ""})
 		// Reflect on the pointed data and check each of its fields.
 		for i := 0; i < v.NumField(); i++ {
 			if !v.Type().Field(i).IsExported() {
 				continue
 			}
-			errs = multierr.Append(errs, validate(v.Field(i)))
+			subpathErrs := validate(v.Field(i))
+			for _, err := range subpathErrs {
+				field := v.Type().Field(i)
+				var fieldName string
+				if tag, ok := field.Tag.Lookup("mapstructure"); ok {
+					tags := strings.Split(tag, ",")
+					if len(tags) > 0 {
+						fieldName = tags[0]
+					}
+				}
+				// Even if the mapstructure tag exists, the field name may not
+				// be available, so set it if it is still blank.
+				if len(fieldName) == 0 {
+					fieldName = strings.ToLower(field.Name)
+				}
+
+				var path string
+				if len(err.path) > 0 {
+					path = strings.Join([]string{fieldName, err.path}, "::")
+				} else {
+					path = fieldName
+				}
+				errs = append(errs, pathError{
+					err:  err.err,
+					path: path,
+				})
+			}
 		}
 		return errs
 	case reflect.Slice, reflect.Array:
-		var errs error
-		errs = multierr.Append(errs, callValidateIfPossible(v))
+		errs = append(errs, pathError{err: callValidateIfPossible(v), path: ""})
 		// Reflect on the pointed data and check each of its fields.
 		for i := 0; i < v.Len(); i++ {
-			errs = multierr.Append(errs, validate(v.Index(i)))
+			subPathErrs := validate(v.Index(i))
+
+			for _, err := range subPathErrs {
+				var path string
+				if len(err.path) > 0 {
+					path = strings.Join([]string{strconv.Itoa(i), err.path}, "::")
+				} else {
+					path = strconv.Itoa(i)
+				}
+
+				errs = append(errs, pathError{
+					err:  err.err,
+					path: path,
+				})
+			}
 		}
 		return errs
 	case reflect.Map:
-		var errs error
-		errs = multierr.Append(errs, callValidateIfPossible(v))
+		errs = append(errs, pathError{err: callValidateIfPossible(v), path: ""})
 		iter := v.MapRange()
 		for iter.Next() {
-			errs = multierr.Append(errs, validate(iter.Key()))
-			errs = multierr.Append(errs, validate(iter.Value()))
+			keyErrs := validate(iter.Key())
+			valueErrs := validate(iter.Value())
+			var key string
+
+			if str, ok := iter.Key().Interface().(string); ok {
+				key = str
+			} else if stringer, ok := iter.Key().Interface().(fmt.Stringer); ok {
+				key = stringer.String()
+			} else {
+				key = fmt.Sprintf("[%T key]", iter.Key().Interface())
+			}
+
+			for _, err := range keyErrs {
+				var path string
+				if len(err.path) > 0 {
+					path = strings.Join([]string{key, err.path}, "::")
+				} else {
+					path = key
+				}
+				errs = append(errs, pathError{err: err.err, path: path})
+			}
+
+			for _, err := range valueErrs {
+				var path string
+				if len(err.path) > 0 {
+					path = strings.Join([]string{key, err.path}, "::")
+				} else {
+					path = key
+				}
+				errs = append(errs, pathError{err: err.err, path: path})
+			}
 		}
 		return errs
 	default:
-		return callValidateIfPossible(v)
+		return []pathError{{err: callValidateIfPossible(v), path: ""}}
 	}
 }
 

component/config_test.go

@@ -5,7 +5,6 @@ package component
 
 import (
 	"errors"
-	"reflect"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -68,11 +67,40 @@ func (e errMapType) Validate() error {
 	return errors.New(e["err"])
 }
 
+type structKey struct {
+	k string
+	e error
+}
+
+func (s structKey) String() string {
+	return s.k
+}
+
+func (s structKey) Validate() error {
+	return s.e
+}
+
+type configChildMapCustomKey struct {
+	Child map[structKey]errConfig
+}
+
 func newErrMapType() *errMapType {
 	et := errMapType(nil)
 	return &et
 }
 
+type configMapstructure struct {
+	Valid  *errConfig `mapstructure:"validtag,omitempty"`
+	NoData *errConfig `mapstructure:""`
+	NoName *errConfig `mapstructure:",remain"`
+}
+
+type configDeeplyNested struct {
+	MapKeyChild   map[configChildStruct]string
+	MapValueChild map[string]configChildStruct
+	SliceChild    []configChildSlice
+}
+
 func TestValidateConfig(t *testing.T) {
 	tests := []struct {
 		name     string
@@ -127,104 +155,144 @@ func TestValidateConfig(t *testing.T) {
 		{
 			name:     "child struct",
 			cfg:      configChildStruct{Child: errConfig{err: errors.New("child struct")}},
-			expected: errors.New("child struct"),
+			expected: errors.New("child: child struct"),
 		},
 		{
 			name:     "pointer child struct",
 			cfg:      &configChildStruct{Child: errConfig{err: errors.New("pointer child struct")}},
-			expected: errors.New("pointer child struct"),
+			expected: errors.New("child: pointer child struct"),
 		},
 		{
 			name:     "child struct pointer",
 			cfg:      &configChildStruct{ChildPtr: &errConfig{err: errors.New("child struct pointer")}},
-			expected: errors.New("child struct pointer"),
+			expected: errors.New("childptr: child struct pointer"),
 		},
 		{
 			name:     "child interface",
 			cfg:      configChildInterface{Child: errConfig{err: errors.New("child interface")}},
-			expected: errors.New("child interface"),
+			expected: errors.New("child: child interface"),
 		},
 		{
 			name:     "pointer to child interface",
 			cfg:      &configChildInterface{Child: errConfig{err: errors.New("pointer to child interface")}},
-			expected: errors.New("pointer to child interface"),
+			expected: errors.New("child: pointer to child interface"),
 		},
 		{
 			name:     "child interface with pointer",
 			cfg:      configChildInterface{Child: &errConfig{err: errors.New("child interface with pointer")}},
-			expected: errors.New("child interface with pointer"),
+			expected: errors.New("child: child interface with pointer"),
 		},
 		{
 			name:     "pointer to child interface with pointer",
 			cfg:      &configChildInterface{Child: &errConfig{err: errors.New("pointer to child interface with pointer")}},
-			expected: errors.New("pointer to child interface with pointer"),
+			expected: errors.New("child: pointer to child interface with pointer"),
 		},
 		{
 			name:     "child slice",
 			cfg:      configChildSlice{Child: []errConfig{{}, {err: errors.New("child slice")}}},
-			expected: errors.New("child slice"),
+			expected: errors.New("child::1: child slice"),
 		},
 		{
 			name:     "pointer child slice",
 			cfg:      &configChildSlice{Child: []errConfig{{}, {err: errors.New("pointer child slice")}}},
-			expected: errors.New("pointer child slice"),
+			expected: errors.New("child::1: pointer child slice"),
 		},
 		{
 			name:     "child slice pointer",
 			cfg:      &configChildSlice{ChildPtr: []*errConfig{{}, {err: errors.New("child slice pointer")}}},
-			expected: errors.New("child slice pointer"),
+			expected: errors.New("childptr::1: child slice pointer"),
 		},
 		{
 			name:     "child map value",
 			cfg:      configChildMapValue{Child: map[string]errConfig{"test": {err: errors.New("child map")}}},
-			expected: errors.New("child map"),
+			expected: errors.New("child::test: child map"),
 		},
 		{
 			name:     "pointer child map value",
 			cfg:      &configChildMapValue{Child: map[string]errConfig{"test": {err: errors.New("pointer child map")}}},
-			expected: errors.New("pointer child map"),
+			expected: errors.New("child::test: pointer child map"),
 		},
 		{
 			name:     "child map value pointer",
 			cfg:      &configChildMapValue{ChildPtr: map[string]*errConfig{"test": {err: errors.New("child map pointer")}}},
-			expected: errors.New("child map pointer"),
+			expected: errors.New("childptr::test: child map pointer"),
 		},
 		{
 			name:     "child map key",
 			cfg:      configChildMapKey{Child: map[errType]string{"child map key": ""}},
-			expected: errors.New("child map key"),
+			expected: errors.New("child::[component.errType key]: child map key"),
 		},
 		{
 			name:     "pointer child map key",
 			cfg:      &configChildMapKey{Child: map[errType]string{"pointer child map key": ""}},
-			expected: errors.New("pointer child map key"),
+			expected: errors.New("child::[component.errType key]: pointer child map key"),
 		},
 		{
 			name:     "child map key pointer",
 			cfg:      &configChildMapKey{ChildPtr: map[*errType]string{newErrType("child map key pointer"): ""}},
-			expected: errors.New("child map key pointer"),
+			expected: errors.New("childptr::[*component.errType key]: child map key pointer"),
+		},
+		{
+			name:     "map with stringified non-string key type",
+			cfg:      &configChildMapCustomKey{Child: map[structKey]errConfig{{k: "struct_key", e: errors.New("custom key error")}: {err: errors.New("value error")}}},
+			expected: errors.New("child::struct_key: custom key error\nchild::struct_key: value error"),
 		},
 		{
 			name:     "child type",
 			cfg:      configChildTypeDef{Child: "child type"},
-			expected: errors.New("child type"),
+			expected: errors.New("child: child type"),
 		},
 		{
 			name:     "pointer child type",
 			cfg:      &configChildTypeDef{Child: "pointer child type"},
-			expected: errors.New("pointer child type"),
+			expected: errors.New("child: pointer child type"),
 		},
 		{
 			name:     "child type pointer",
 			cfg:      &configChildTypeDef{ChildPtr: newErrType("child type pointer")},
-			expected: errors.New("child type pointer"),
+			expected: errors.New("childptr: child type pointer"),
+		},
+		{
+			name:     "valid mapstructure tag",
+			cfg:      configMapstructure{Valid: &errConfig{errors.New("test")}},
+			expected: errors.New("validtag: test"),
+		},
+		{
+			name:     "zero-length mapstructure tag",
+			cfg:      configMapstructure{NoData: &errConfig{errors.New("test")}},
+			expected: errors.New("nodata: test"),
+		},
+		{
+			name:     "no field name in mapstructure tag",
+			cfg:      configMapstructure{NoName: &errConfig{errors.New("test")}},
+			expected: errors.New("noname: test"),
+		},
+		{
+			name:     "nested map key error",
+			cfg:      configDeeplyNested{MapKeyChild: map[configChildStruct]string{{Child: errConfig{err: errors.New("child key error")}}: "val"}},
+			expected: errors.New("mapkeychild::[component.configChildStruct key]::child: child key error"),
+		},
+		{
+			name:     "nested map value error",
+			cfg:      configDeeplyNested{MapValueChild: map[string]configChildStruct{"key": {Child: errConfig{err: errors.New("child key error")}}}},
+			expected: errors.New("mapvaluechild::key::child: child key error"),
+		},
+		{
+			name:     "nested slice value error",
+			cfg:      configDeeplyNested{SliceChild: []configChildSlice{{Child: []errConfig{{err: errors.New("child key error")}}}}},
+			expected: errors.New("slicechild::0::child::0: child key error"),
 		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			err := validate(reflect.ValueOf(tt.cfg))
-			assert.Equal(t, tt.expected, err)
+			err := ValidateConfig(tt.cfg)
+
+			if tt.expected != nil {
+				assert.EqualError(t, err, tt.expected.Error())
+			} else {
+				assert.NoError(t, err)
+			}
 		})
 	}
 }