From 38d4974de7657f81d2d9778a97b07a6c4b29051a Mon Sep 17 00:00:00 2001 From: Evan Bradley Date: Thu, 16 Jan 2025 14:50:38 -0500 Subject: [PATCH 1/9] [component] Print path to error in `ValidateConfig` --- component/config.go | 118 ++++++++++++++++++++++++++++++++++----- component/config_test.go | 112 +++++++++++++++++++++++++++++-------- 2 files changed, 193 insertions(+), 37 deletions(-) diff --git a/component/config.go b/component/config.go index 11cbf7dabc8..038103aec5a 100644 --- a/component/config.go +++ b/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,10 +33,29 @@ 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: @@ -42,35 +63,102 @@ func validate(v reflect.Value) error { 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: ""}} } } diff --git a/component/config_test.go b/component/config_test.go index c3b316c2a94..4157339b2c8 100644 --- a/component/config_test.go +++ b/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.Nil(t, err) + } }) } } From 96dd9d363c8dcfc1fd40ef3d37a79affeaf564ab Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Fri, 24 Jan 2025 15:06:50 -0500 Subject: [PATCH 2/9] Add changelog --- .chloggen/validateconfig-print-path.yaml | 25 ++++++++++++++++++++++++ 1 file changed, 25 insertions(+) create mode 100644 .chloggen/validateconfig-print-path.yaml diff --git a/.chloggen/validateconfig-print-path.yaml b/.chloggen/validateconfig-print-path.yaml new file mode 100644 index 00000000000..230c5377f3e --- /dev/null +++ b/.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] From e2143f10ed36705f297f5af8eec166ec0da84061 Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Fri, 24 Jan 2025 15:09:53 -0500 Subject: [PATCH 3/9] go mod tidy --- component/go.mod | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/component/go.mod b/component/go.mod index 7057576920a..b158709f4b2 100644 --- a/component/go.mod +++ b/component/go.mod @@ -9,7 +9,6 @@ require ( go.opentelemetry.io/otel/metric v1.34.0 go.opentelemetry.io/otel/trace v1.34.0 go.uber.org/goleak v1.3.0 - go.uber.org/multierr v1.11.0 go.uber.org/zap v1.27.0 ) @@ -18,6 +17,7 @@ require ( github.com/gogo/protobuf v1.3.2 // indirect github.com/pmezard/go-difflib v1.0.0 // indirect go.opentelemetry.io/otel v1.34.0 // indirect + go.uber.org/multierr v1.11.0 // indirect golang.org/x/net v0.33.0 // indirect golang.org/x/sys v0.28.0 // indirect golang.org/x/text v0.21.0 // indirect From 397bfa1fbb22fbbbd514358a8ab7cc5218d646ce Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Fri, 24 Jan 2025 15:10:29 -0500 Subject: [PATCH 4/9] lint --- component/config.go | 1 - component/config_test.go | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/component/config.go b/component/config.go index 038103aec5a..d70012a6de5 100644 --- a/component/config.go +++ b/component/config.go @@ -117,7 +117,6 @@ func validate(v reflect.Value) []pathError { path: path, }) } - } return errs case reflect.Map: diff --git a/component/config_test.go b/component/config_test.go index 4157339b2c8..e8aa86c02c6 100644 --- a/component/config_test.go +++ b/component/config_test.go @@ -291,7 +291,7 @@ func TestValidateConfig(t *testing.T) { if tt.expected != nil { assert.EqualError(t, err, tt.expected.Error()) } else { - assert.Nil(t, err) + assert.NoError(t, err) } }) } From 242d16eba8bba64ae8940ebb21e18b814f2fcead Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Fri, 24 Jan 2025 15:21:33 -0500 Subject: [PATCH 5/9] Update otelcol test --- otelcol/config_test.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/otelcol/config_test.go b/otelcol/config_test.go index ce46ab048f5..3dc204d9267 100644 --- a/otelcol/config_test.go +++ b/otelcol/config_test.go @@ -241,7 +241,12 @@ func TestConfigValidate(t *testing.T) { for _, tt := range testCases { t.Run(tt.name, func(t *testing.T) { cfg := tt.cfgFn() - assert.Equal(t, tt.expected, cfg.Validate()) + err := cfg.Validate() + if tt.expected != nil { + assert.EqualError(t, err, tt.expected.Error()) + } else { + assert.NoError(t, err) + } }) } } From 20a87a0311c5b70e43007576ac116c2cc08246d0 Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Wed, 29 Jan 2025 11:09:49 -0500 Subject: [PATCH 6/9] Refactor --- component/config.go | 120 +++++++++++++++++++++------------------ component/config_test.go | 11 ++++ 2 files changed, 76 insertions(+), 55 deletions(-) diff --git a/component/config.go b/component/config.go index d70012a6de5..5f40725c0bc 100644 --- a/component/config.go +++ b/component/config.go @@ -37,13 +37,7 @@ func ValidateConfig(cfg Config) 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) - } - } + err = errors.Join(err, suberr.toError()) } return err @@ -51,7 +45,25 @@ func ValidateConfig(cfg Config) error { type pathError struct { err error - path string + path []string +} + +func (pe pathError) toError() error { + if len(pe.path) > 0 { + var path string + sb := strings.Builder{} + + _, _ = sb.WriteString(pe.path[len(pe.path)-1]) + for i := len(pe.path) - 2; i >= 0; i-- { + _, _ = sb.WriteString("::") + _, _ = sb.WriteString(pe.path[i]) + } + path = sb.String() + + return fmt.Errorf("%s: %w", path, pe.err) + } + + return pe.err } func validate(v reflect.Value) []pathError { @@ -63,64 +75,52 @@ func validate(v reflect.Value) []pathError { case reflect.Ptr, reflect.Interface: return validate(v.Elem()) case reflect.Struct: - errs = append(errs, pathError{err: callValidateIfPossible(v), path: ""}) + err := callValidateIfPossible(v) + if err != nil { + errs = append(errs, pathError{err: err}) + } + // 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 } + field := v.Type().Field(i) + path := fieldName(field) + 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, + path: append(err.path, path), }) } } return errs case reflect.Slice, reflect.Array: - errs = append(errs, pathError{err: callValidateIfPossible(v), path: ""}) + err := callValidateIfPossible(v) + if err != nil { + errs = append(errs, pathError{err: err}) + } + // Reflect on the pointed data and check each of its fields. for i := 0; i < v.Len(); 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, + path: append(err.path, strconv.Itoa(i)), }) } } return errs case reflect.Map: - errs = append(errs, pathError{err: callValidateIfPossible(v), path: ""}) + err := callValidateIfPossible(v) + if err != nil { + errs = append(errs, pathError{err: err}) + } + iter := v.MapRange() for iter.Next() { keyErrs := validate(iter.Key()) @@ -136,28 +136,21 @@ func validate(v reflect.Value) []pathError { } 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}) + errs = append(errs, pathError{err: err.err, path: append(err.path, key)}) } 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}) + errs = append(errs, pathError{err: err.err, path: append(err.path, key)}) } } return errs default: - return []pathError{{err: callValidateIfPossible(v), path: ""}} + err := callValidateIfPossible(v) + if err != nil { + return []pathError{{err: err}} + } + + return nil } } @@ -180,3 +173,20 @@ func callValidateIfPossible(v reflect.Value) error { return nil } + +func fieldName(field reflect.StructField) string { + 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) + } + + return fieldName +} diff --git a/component/config_test.go b/component/config_test.go index e8aa86c02c6..d1b36cf9a00 100644 --- a/component/config_test.go +++ b/component/config_test.go @@ -101,6 +101,12 @@ type configDeeplyNested struct { SliceChild []configChildSlice } +type sliceTypeAlias []configChildSlice + +func (sliceTypeAlias) Validate() error { + return errors.New("sliceTypeAlias error") +} + func TestValidateConfig(t *testing.T) { tests := []struct { name string @@ -282,6 +288,11 @@ func TestValidateConfig(t *testing.T) { cfg: configDeeplyNested{SliceChild: []configChildSlice{{Child: []errConfig{{err: errors.New("child key error")}}}}}, expected: errors.New("slicechild::0::child::0: child key error"), }, + { + name: "slice type alias", + cfg: sliceTypeAlias{}, + expected: errors.New("sliceTypeAlias error"), + }, } for _, tt := range tests { From b746de26e8458f44883f5f191a2d8994616267e3 Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Wed, 29 Jan 2025 11:29:26 -0500 Subject: [PATCH 7/9] Coerce primitive key types to values --- component/config.go | 29 ++++++++++++++++++++--------- component/config_test.go | 20 ++++++++++++++++---- 2 files changed, 36 insertions(+), 13 deletions(-) diff --git a/component/config.go b/component/config.go index 5f40725c0bc..064e1c7f44d 100644 --- a/component/config.go +++ b/component/config.go @@ -125,15 +125,7 @@ func validate(v reflect.Value) []pathError { for iter.Next() { 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()) - } + key := stringifyMapKey(iter.Key()) for _, err := range keyErrs { errs = append(errs, pathError{err: err.err, path: append(err.path, key)}) @@ -190,3 +182,22 @@ func fieldName(field reflect.StructField) string { return fieldName } + +func stringifyMapKey(val reflect.Value) string { + var key string + + if str, ok := val.Interface().(string); ok { + key = str + } else if stringer, ok := val.Interface().(fmt.Stringer); ok { + key = stringer.String() + } else { + switch val.Kind() { + case reflect.Ptr, reflect.Interface, reflect.Struct, reflect.Slice, reflect.Array, reflect.Map: + key = fmt.Sprintf("[%T key]", val.Interface()) + default: + key = fmt.Sprintf("%v", val.Interface()) + } + } + + return key +} diff --git a/component/config_test.go b/component/config_test.go index d1b36cf9a00..686c0fb7d98 100644 --- a/component/config_test.go +++ b/component/config_test.go @@ -99,6 +99,8 @@ type configDeeplyNested struct { MapKeyChild map[configChildStruct]string MapValueChild map[string]configChildStruct SliceChild []configChildSlice + MapIntKey map[int]errConfig + MapFloatKey map[float64]errConfig } type sliceTypeAlias []configChildSlice @@ -225,13 +227,13 @@ func TestValidateConfig(t *testing.T) { }, { name: "child map key", - cfg: configChildMapKey{Child: map[errType]string{"child map key": ""}}, - expected: errors.New("child::[component.errType key]: child map key"), + cfg: configChildMapKey{Child: map[errType]string{"child_map_key": ""}}, + expected: errors.New("child::child_map_key: child_map_key"), }, { name: "pointer child map key", - cfg: &configChildMapKey{Child: map[errType]string{"pointer child map key": ""}}, - expected: errors.New("child::[component.errType key]: pointer child map key"), + cfg: &configChildMapKey{Child: map[errType]string{"pointer_child_map_key": ""}}, + expected: errors.New("child::pointer_child_map_key: pointer_child_map_key"), }, { name: "child map key pointer", @@ -288,6 +290,16 @@ func TestValidateConfig(t *testing.T) { cfg: configDeeplyNested{SliceChild: []configChildSlice{{Child: []errConfig{{err: errors.New("child key error")}}}}}, expected: errors.New("slicechild::0::child::0: child key error"), }, + { + name: "nested map with int key", + cfg: configDeeplyNested{MapIntKey: map[int]errConfig{1: {err: errors.New("int key error")}}}, + expected: errors.New("mapintkey::1: int key error"), + }, + { + name: "nested map with float key", + cfg: configDeeplyNested{MapFloatKey: map[float64]errConfig{1.2: {err: errors.New("float key error")}}}, + expected: errors.New("mapfloatkey::1.2: float key error"), + }, { name: "slice type alias", cfg: sliceTypeAlias{}, From f075724c0310845d8b10569fe4018c6f94f3fdc8 Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Wed, 29 Jan 2025 12:06:27 -0500 Subject: [PATCH 8/9] Apply suggestions from code review Co-authored-by: Pablo Baeyens --- component/config.go | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/component/config.go b/component/config.go index 064e1c7f44d..2b30c7564e3 100644 --- a/component/config.go +++ b/component/config.go @@ -33,14 +33,7 @@ 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 { - var err error - errs := validate(reflect.ValueOf(cfg)) - - for _, suberr := range errs { - err = errors.Join(err, suberr.toError()) - } - - return err + return errors.Join(validate(reflect.ValueOf(cfg))) } type pathError struct { @@ -48,7 +41,7 @@ type pathError struct { path []string } -func (pe pathError) toError() error { +func (pe pathError) Error() string { if len(pe.path) > 0 { var path string sb := strings.Builder{} @@ -60,9 +53,13 @@ func (pe pathError) toError() error { } path = sb.String() - return fmt.Errorf("%s: %w", path, pe.err) + return fmt.Sprintf("%s: %s", path, pe.err) } + return pe.err.Error() +} + +func (pe pathError) Unwrap() error { return pe.err } From 7477cf0cdde6978dc790583f09c27d0c0e4c85b8 Mon Sep 17 00:00:00 2001 From: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Date: Wed, 29 Jan 2025 12:21:05 -0500 Subject: [PATCH 9/9] Loop over errors again --- component/config.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/component/config.go b/component/config.go index 2b30c7564e3..a5862798fdd 100644 --- a/component/config.go +++ b/component/config.go @@ -33,7 +33,13 @@ 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 errors.Join(validate(reflect.ValueOf(cfg))) + var err error + + for _, validationErr := range validate(reflect.ValueOf(cfg)) { + err = errors.Join(err, validationErr) + } + + return err } type pathError struct {