Skip to content

Commit bcbaeaf

Browse files
alexperez52Hyunuk LimAneurysm9
authored
Move ValidateConfig from configcheck to configtest (#3956)
* Move Validateconfig to configtest * Rename ValidateConfig to CheckConfigStruct * fix: revert all the renamed variables * fix: rename ValidateConfig to CheckConfigStruct * fix: rename test names Co-authored-by: Hyunuk Lim <[email protected]> Co-authored-by: Anthony Mirabella <[email protected]>
1 parent edfde72 commit bcbaeaf

File tree

17 files changed

+300
-299
lines changed

17 files changed

+300
-299
lines changed

component/experimental/component/factory.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ type ConfigSourceFactory interface {
4444
// configuration and should not cause side-effects that prevent the creation
4545
// of multiple instances of the Source.
4646
// The object returned by this method needs to pass the checks implemented by
47-
// 'configcheck.ValidateConfig'. It is recommended to have such check in the
47+
// 'configtest.CheckConfigStruct'. It is recommended to have such check in the
4848
// tests of any implementation of the ConfigSourceFactory interface.
4949
CreateDefaultConfig() config.Source
5050

component/exporter.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@ type ExporterFactory interface {
6262
// configuration and should not cause side-effects that prevent the creation
6363
// of multiple instances of the Exporter.
6464
// The object returned by this method needs to pass the checks implemented by
65-
// 'configcheck.ValidateConfig'. It is recommended to have these checks in the
65+
// 'configtest.CheckConfigStruct'. It is recommended to have these checks in the
6666
// tests of any implementation of the Factory interface.
6767
CreateDefaultConfig() config.Exporter
6868

component/extension.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ type ExtensionFactory interface {
6161
// configuration and should not cause side-effects that prevent the creation
6262
// of multiple instances of the Extension.
6363
// The object returned by this method needs to pass the checks implemented by
64-
// 'configcheck.ValidateConfig'. It is recommended to have these checks in the
64+
// 'configtest.CheckConfigStruct'. It is recommended to have these checks in the
6565
// tests of any implementation of the Factory interface.
6666
CreateDefaultConfig() config.Extension
6767

component/processor.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,7 @@ type ProcessorFactory interface {
6767
// configuration and should not cause side-effects that prevent the creation
6868
// of multiple instances of the Processor.
6969
// The object returned by this method needs to pass the checks implemented by
70-
// 'configcheck.ValidateConfig'. It is recommended to have these checks in the
70+
// 'configtest.CheckConfigStruct'. It is recommended to have these checks in the
7171
// tests of any implementation of the Factory interface.
7272
CreateDefaultConfig() config.Processor
7373

component/receiver.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ type ReceiverFactory interface {
7171
// configuration and should not cause side-effects that prevent the creation
7272
// of multiple instances of the Receiver.
7373
// The object returned by this method needs to pass the checks implemented by
74-
// 'configcheck.ValidateConfig'. It is recommended to have these checks in the
74+
// 'configtest.CheckConfigStruct'. It is recommended to have these checks in the
7575
// tests of any implementation of the Factory interface.
7676
CreateDefaultConfig() config.Receiver
7777

config/configcheck/configcheck.go

+5-140
Original file line numberDiff line numberDiff line change
@@ -15,171 +15,36 @@
1515
package configcheck
1616

1717
import (
18-
"fmt"
19-
"reflect"
20-
"regexp"
21-
"strings"
22-
2318
"go.opentelemetry.io/collector/component"
19+
"go.opentelemetry.io/collector/config/configtest"
2420
"go.opentelemetry.io/collector/consumer/consumererror"
2521
)
2622

27-
// The regular expression for valid config field tag.
28-
var configFieldTagRegExp = regexp.MustCompile("^[a-z0-9][a-z0-9_]*$")
29-
3023
// ValidateConfigFromFactories checks if all configurations for the given factories
3124
// are satisfying the patterns used by the collector.
3225
func ValidateConfigFromFactories(factories component.Factories) error {
3326
var errs []error
3427

3528
for _, factory := range factories.Receivers {
36-
if err := ValidateConfig(factory.CreateDefaultConfig()); err != nil {
29+
if err := configtest.CheckConfigStruct(factory.CreateDefaultConfig()); err != nil {
3730
errs = append(errs, err)
3831
}
3932
}
4033
for _, factory := range factories.Processors {
41-
if err := ValidateConfig(factory.CreateDefaultConfig()); err != nil {
34+
if err := configtest.CheckConfigStruct(factory.CreateDefaultConfig()); err != nil {
4235
errs = append(errs, err)
4336
}
4437
}
4538
for _, factory := range factories.Exporters {
46-
if err := ValidateConfig(factory.CreateDefaultConfig()); err != nil {
39+
if err := configtest.CheckConfigStruct(factory.CreateDefaultConfig()); err != nil {
4740
errs = append(errs, err)
4841
}
4942
}
5043
for _, factory := range factories.Extensions {
51-
if err := ValidateConfig(factory.CreateDefaultConfig()); err != nil {
44+
if err := configtest.CheckConfigStruct(factory.CreateDefaultConfig()); err != nil {
5245
errs = append(errs, err)
5346
}
5447
}
5548

5649
return consumererror.Combine(errs)
5750
}
58-
59-
// ValidateConfig enforces that given configuration object is following the patterns
60-
// used by the collector. This ensures consistency between different implementations
61-
// of components and extensions. It is recommended for implementers of components
62-
// to call this function on their tests passing the default configuration of the
63-
// component factory.
64-
func ValidateConfig(config interface{}) error {
65-
t := reflect.TypeOf(config)
66-
if t.Kind() == reflect.Ptr {
67-
t = t.Elem()
68-
}
69-
70-
if t.Kind() != reflect.Struct {
71-
return fmt.Errorf("config must be a struct or a pointer to one, the passed object is a %s", t.Kind())
72-
}
73-
74-
return validateConfigDataType(t)
75-
}
76-
77-
// validateConfigDataType performs a descending validation of the given type.
78-
// If the type is a struct it goes to each of its fields to check for the proper
79-
// tags.
80-
func validateConfigDataType(t reflect.Type) error {
81-
var errs []error
82-
83-
switch t.Kind() {
84-
case reflect.Ptr:
85-
if err := validateConfigDataType(t.Elem()); err != nil {
86-
errs = append(errs, err)
87-
}
88-
case reflect.Struct:
89-
// Reflect on the pointed data and check each of its fields.
90-
nf := t.NumField()
91-
for i := 0; i < nf; i++ {
92-
f := t.Field(i)
93-
if err := checkStructFieldTags(f); err != nil {
94-
errs = append(errs, err)
95-
}
96-
}
97-
default:
98-
// The config object can carry other types but they are not used when
99-
// reading the configuration via koanf so ignore them. Basically ignore:
100-
// reflect.Uintptr, reflect.Chan, reflect.Func, reflect.Interface, and
101-
// reflect.UnsafePointer.
102-
}
103-
104-
if err := consumererror.Combine(errs); err != nil {
105-
return fmt.Errorf(
106-
"type %q from package %q has invalid config settings: %v",
107-
t.Name(),
108-
t.PkgPath(),
109-
err)
110-
}
111-
112-
return nil
113-
}
114-
115-
// checkStructFieldTags inspects the tags of a struct field.
116-
func checkStructFieldTags(f reflect.StructField) error {
117-
118-
tagValue := f.Tag.Get("mapstructure")
119-
if tagValue == "" {
120-
121-
// Ignore special types.
122-
switch f.Type.Kind() {
123-
case reflect.Interface, reflect.Chan, reflect.Func, reflect.Uintptr, reflect.UnsafePointer:
124-
// Allow the config to carry the types above, but since they are not read
125-
// when loading configuration, just ignore them.
126-
return nil
127-
}
128-
129-
// Public fields of other types should be tagged.
130-
chars := []byte(f.Name)
131-
if len(chars) > 0 && chars[0] >= 'A' && chars[0] <= 'Z' {
132-
return fmt.Errorf("mapstructure tag not present on field %q", f.Name)
133-
}
134-
135-
// Not public field, no need to have a tag.
136-
return nil
137-
}
138-
139-
tagParts := strings.Split(tagValue, ",")
140-
if tagParts[0] != "" {
141-
if tagParts[0] == "-" {
142-
// Nothing to do, as mapstructure decode skips this field.
143-
return nil
144-
}
145-
}
146-
147-
// Check if squash is specified.
148-
squash := false
149-
for _, tag := range tagParts[1:] {
150-
if tag == "squash" {
151-
squash = true
152-
break
153-
}
154-
}
155-
156-
if squash {
157-
// Field was squashed.
158-
if (f.Type.Kind() != reflect.Struct) && (f.Type.Kind() != reflect.Ptr || f.Type.Elem().Kind() != reflect.Struct) {
159-
return fmt.Errorf(
160-
"attempt to squash non-struct type on field %q", f.Name)
161-
}
162-
}
163-
164-
switch f.Type.Kind() {
165-
case reflect.Struct:
166-
// It is another struct, continue down-level.
167-
return validateConfigDataType(f.Type)
168-
169-
case reflect.Map, reflect.Slice, reflect.Array:
170-
// The element of map, array, or slice can be itself a configuration object.
171-
return validateConfigDataType(f.Type.Elem())
172-
173-
default:
174-
fieldTag := tagParts[0]
175-
if !configFieldTagRegExp.MatchString(fieldTag) {
176-
return fmt.Errorf(
177-
"field %q has config tag %q which doesn't satisfy %q",
178-
f.Name,
179-
fieldTag,
180-
configFieldTagRegExp.String())
181-
}
182-
}
183-
184-
return nil
185-
}

config/configcheck/configcheck_test.go

-138
Original file line numberDiff line numberDiff line change
@@ -16,11 +16,8 @@ package configcheck
1616

1717
import (
1818
"context"
19-
"io"
20-
"strings"
2119
"testing"
2220

23-
"github.com/stretchr/testify/assert"
2421
"github.com/stretchr/testify/require"
2522

2623
"go.opentelemetry.io/collector/component"
@@ -48,141 +45,6 @@ func TestValidateConfigFromFactories_Failure(t *testing.T) {
4845
require.Error(t, err)
4946
}
5047

51-
func TestValidateConfigPointerAndValue(t *testing.T) {
52-
config := struct {
53-
SomeFiled string `mapstructure:"test"`
54-
}{}
55-
assert.NoError(t, ValidateConfig(config))
56-
assert.NoError(t, ValidateConfig(&config))
57-
}
58-
59-
func TestValidateConfig(t *testing.T) {
60-
type BadConfigTag struct {
61-
BadTagField int `mapstructure:"test-dash"`
62-
}
63-
64-
tests := []struct {
65-
name string
66-
config interface{}
67-
wantErrMsgSubStr string
68-
}{
69-
{
70-
name: "typical_config",
71-
config: struct {
72-
MyPublicString string `mapstructure:"string"`
73-
}{},
74-
},
75-
{
76-
name: "private_fields_ignored",
77-
config: struct {
78-
// A public type with proper tag.
79-
MyPublicString string `mapstructure:"string"`
80-
// A public type with proper tag.
81-
MyPublicInt string `mapstructure:"int"`
82-
// A public type that should be ignored.
83-
MyFunc func() error
84-
// A public type that should be ignored.
85-
Reader io.Reader
86-
// private type not tagged.
87-
myPrivateString string
88-
_someInt int
89-
}{},
90-
},
91-
{
92-
name: "not_struct_nor_pointer",
93-
config: func(x int) int {
94-
return x * x
95-
},
96-
wantErrMsgSubStr: "config must be a struct or a pointer to one, the passed object is a func",
97-
},
98-
{
99-
name: "squash_on_non_struct",
100-
config: struct {
101-
MyInt int `mapstructure:",squash"`
102-
}{},
103-
wantErrMsgSubStr: "attempt to squash non-struct type on field \"MyInt\"",
104-
},
105-
{
106-
name: "invalid_tag_detected",
107-
config: BadConfigTag{},
108-
wantErrMsgSubStr: "field \"BadTagField\" has config tag \"test-dash\" which doesn't satisfy",
109-
},
110-
{
111-
name: "public_field_must_have_tag",
112-
config: struct {
113-
PublicFieldWithoutMapstructureTag string
114-
}{},
115-
wantErrMsgSubStr: "mapstructure tag not present on field \"PublicFieldWithoutMapstructureTag\"",
116-
},
117-
{
118-
name: "invalid_map_item",
119-
config: struct {
120-
Map map[string]BadConfigTag `mapstructure:"test_map"`
121-
}{},
122-
wantErrMsgSubStr: "field \"BadTagField\" has config tag \"test-dash\" which doesn't satisfy",
123-
},
124-
{
125-
name: "invalid_slice_item",
126-
config: struct {
127-
Slice []BadConfigTag `mapstructure:"test_slice"`
128-
}{},
129-
wantErrMsgSubStr: "field \"BadTagField\" has config tag \"test-dash\" which doesn't satisfy",
130-
},
131-
{
132-
name: "invalid_array_item",
133-
config: struct {
134-
Array [2]BadConfigTag `mapstructure:"test_array"`
135-
}{},
136-
wantErrMsgSubStr: "field \"BadTagField\" has config tag \"test-dash\" which doesn't satisfy",
137-
},
138-
{
139-
name: "invalid_map_item_ptr",
140-
config: struct {
141-
Map map[string]*BadConfigTag `mapstructure:"test_map"`
142-
}{},
143-
wantErrMsgSubStr: "field \"BadTagField\" has config tag \"test-dash\" which doesn't satisfy",
144-
},
145-
{
146-
name: "invalid_slice_item_ptr",
147-
config: struct {
148-
Slice []*BadConfigTag `mapstructure:"test_slice"`
149-
}{},
150-
wantErrMsgSubStr: "field \"BadTagField\" has config tag \"test-dash\" which doesn't satisfy",
151-
},
152-
{
153-
name: "invalid_array_item_ptr",
154-
config: struct {
155-
Array [2]*BadConfigTag `mapstructure:"test_array"`
156-
}{},
157-
wantErrMsgSubStr: "field \"BadTagField\" has config tag \"test-dash\" which doesn't satisfy",
158-
},
159-
{
160-
name: "valid_map_item",
161-
config: struct {
162-
Map map[string]int `mapstructure:"test_map"`
163-
}{},
164-
},
165-
{
166-
name: "valid_slice_item",
167-
config: struct {
168-
Slice []string `mapstructure:"test_slice"`
169-
}{},
170-
},
171-
}
172-
173-
for _, tt := range tests {
174-
t.Run(tt.name, func(t *testing.T) {
175-
err := ValidateConfig(tt.config)
176-
if tt.wantErrMsgSubStr == "" {
177-
assert.NoError(t, err)
178-
} else {
179-
require.Error(t, err)
180-
assert.True(t, strings.Contains(err.Error(), tt.wantErrMsgSubStr))
181-
}
182-
})
183-
}
184-
}
185-
18648
// badConfigExtensionFactory was created to force error path from factory returning
18749
// a config not satisfying the validation.
18850
type badConfigExtensionFactory struct{}

0 commit comments

Comments
 (0)